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

Manage script including arm virtualisation #1388

Merged
merged 13 commits into from
Oct 16, 2024
Merged

Manage script including arm virtualisation #1388

merged 13 commits into from
Oct 16, 2024

Conversation

H4LL
Copy link
Contributor

@H4LL H4LL commented Oct 10, 2024

I made a PR the other dayh about mac virtualisation: #1371 (review)

I ended up turning it into a ./manage file. I hadn't originally included signoffs in my previous PR so I thought I'd amend it but I ended up touching a bunch of commits which were nothing to do with me. I thought it would be simpler to create a fresh PR here.

I tested this by removing all images then running through ./manage build, stop, start, restart.

Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
@H4LL H4LL mentioned this pull request Oct 10, 2024
Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
@WadeBarnes
Copy link
Member

I'm assuming #1371 can be closed in favor of this PR. Is that correct?

scripts/manage Outdated Show resolved Hide resolved
scripts/manage Outdated Show resolved Hide resolved
Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
@H4LL
Copy link
Contributor Author

H4LL commented Oct 11, 2024

Thanks for the feedback, I went ahead and closed that other PR. I've also added a UI to the manage script to make it a little nicer in db33f52

Screenshot 2024-10-11 at 11 00 45 AM

Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
Copy link
Member

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! In addition to the stop, I would recommend including a down | rm command to allow a user to reset their environment. My previous comment regarding the the down and stop options was meant to recommend the addition of the stop command.

No a show stopper, it could be added later.

WadeBarnes
WadeBarnes previously approved these changes Oct 11, 2024
@loneil
Copy link
Contributor

loneil commented Oct 11, 2024

This looks great, my one thing with the ./manage scripts (which we do use in plenty of our projects so is a good idea to match in Traction here) is that if I'm on Windows and just pull the repo and follow the instructions nothing will happen so it can look like it's broken. I can use Git Bash or WSL. But with docker compose build/up I can just run in powershell/cmd.

Probably just best to cover that with some notes in the Readme, unless anyone else has other thoughts on using Unix-style scripts for starting up here?

Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @loneil about providing a little bit more detail on the fact that the script needs to be run in a bash-compatible shell as it might not be obvious for everyone running the project for the first time. Other than that this looks great!

I'd rather have it added in this PR than a follow-up just so that the changes are self-contained (and since we already tweaked the instructions in the readme file).

Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
Signed-off-by: Adam J Hall <46713492+H4LL@users.noreply.github.com>
@H4LL
Copy link
Contributor Author

H4LL commented Oct 14, 2024

Hey @esune @loneil @WadeBarnes,

Thanks, I've updated the manage script to have a down option and I've reworded the instruction in the README to be explicit about using a bash shell. The updates are in 344346a and 31edeb6 respectively.

Let me know if there's anything else you'd like!

Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, used Win11 and Git Bash, ran with ./manage no args and used prompts to build and start.

I usually check logs outside of the terminal I would use to start up so not a big difference to me, but noticed in the older docker compose up instructions you would see the logs for all the containers tailing out, whereas with manage the way I ran it it sits like this

image

when we use the manage script on VCAuthN for example we have logs tail out

image

if anyone else has thoughts on if keeping that pattern is important?

@H4LL
Copy link
Contributor Author

H4LL commented Oct 16, 2024

Hey @loneil,

I could explore a 'logs' option which could potentially overwrite the default behavior of the terminal for easier debugging. I'm sure it wont take too long (famous last words). If we want to go deep with that option I could also try a logs dir where the logs from each container are directed into their own file. I've not seen that design pattern in any other Aries adjacent repos but I feel like it'd be useful.

@loneil
Copy link
Contributor

loneil commented Oct 16, 2024

Hey @loneil,

I could explore a 'logs' option which could potentially overwrite the default behavior of the terminal for easier debugging. I'm sure it wont take too long (famous last words). If we want to go deep with that option I could also try a logs dir where the logs from each container are directed into their own file. I've not seen that design pattern in any other Aries adjacent repos but I feel like it'd be useful.

Hi @H4LL I think it's useful enough to get what we have here in and look at logging enhancement as a separate issue. I'll create one for that later and link it back. So going to approve this and merge it shortly unless anyone else has any thoughts.

Other ones we use on our team that just tail to the terminal are
https://github.com/bcgov/vc-authn-oidc/blob/main/docker/manage
https://github.com/hyperledger/aries-endorser-service/blob/main/docker/manage

but yeah I think there's some other patterns around other Aries apps as well.

Thanks!

@loneil loneil merged commit 53ddad5 into bcgov:main Oct 16, 2024
1 check passed
@loneil
Copy link
Contributor

loneil commented Oct 16, 2024

Issue to look into log output created: #1401

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