Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DO NOT MERGE - Per Tenant Logging Testing #875

Closed
wants to merge 10 commits into from

Conversation

shaangill025
Copy link
Contributor

No description provided.

Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
…r_tenant_logging

Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
@esune
Copy link
Member

esune commented Oct 23, 2023

@shaangill025 the permission error we are seeing when creating the log file in OpenShift could be resolved by having the logs go into /home/aries/log rather than in /home/aries, as that folder should have the right privileges set for the user to write to it.
We probably want to have this applied as default folder in ACA-Py, rather than leaving it to the user.

@shaangill025
Copy link
Contributor Author

@esune I made the change in the following commit to allow --log-file to be applied to the logging handler if already created from config. So --log-file ./log/acapy.log would work with new pre-release image. Earlier the path specified in the config [ini] file could not be override. Regarding putting ./log/.. as the default path in ACA-Py, I am not sure if this is the way to go as this would work within Traction env but not if ACA-Py [multitenant] is stood up by itself. Currently the hang up is with the caplog issue with unit tests as I had mentioned in my updates today.
openwallet-foundation/acapy@c46c4d0

@esune
Copy link
Member

esune commented Oct 23, 2023

@esune I made the change in the following commit to allow --log-file to be applied to the logging handler if already created from config. So --log-file ./log/acapy.log would work with new pre-release image. Earlier the path specified in the config [ini] file could not be override. Regarding putting ./log/.. as the default path in ACA-Py, I am not sure if this is the way to go as this would work within Traction env but not if ACA-Py [multitenant] is stood up by itself. Currently the hang up is with the caplog issue with unit tests as I had mentioned in my updates today. hyperledger/aries-cloudagent-python@c46c4d0

Thank you @shaangill025, i think I follow now. Looping in @WadeBarnes for potential input/insight on the operations end.

@WadeBarnes
Copy link
Member

For anyone using the ACA-Py images the ./log folder is where logs are expected to be written if stored to disk.

@swcurran
Copy link
Contributor

@WadeBarnes — are you now happy with the implementation in ACA-Py? As in approving the PR?

@shaangill025 — two of the test Actions are failing in the ACA-Py PR. Could you please investigate and address them? We’ve had some trouble with the JSON-LD tests failing to resolve a URL that could be the cause of the integration test errors (or not…), but that would not be the issue with the other failing action.

@WadeBarnes
Copy link
Member

@WadeBarnes — are you now happy with the implementation in ACA-Py? As in approving the PR?

I've been letting @esune lead on the review of the Per Tenant Logging. I'm just commenting on the default location used for ACA-Py when it logs to disk. The images setup a ./log (more precisely a ~/log) folder and relevant permissions for this purpose, but the defaults in ACA-Py seem to log to files in the . (more precisely the ~) folder.

@WadeBarnes
Copy link
Member

The pods associated with this PR are still in a crash loop.
cc @shaangill025, @esune, @i5okie

Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
@shaangill025 shaangill025 temporarily deployed to development October 25, 2023 19:59 — with GitHub Actions Inactive
@esune
Copy link
Member

esune commented Nov 27, 2023

@shaangill025 do we need to do more testing on this or are we good since the changes were incorporated in ACA-Py and we can therefore close the PR?

@shaangill025
Copy link
Contributor Author

We can close this PR as the associated changes are already merged in ACA-Py.

@loneil loneil deleted the test_per_tenant_logging branch June 11, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants