-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add a script to create the Quickstart package #612
Add a script to create the Quickstart package #612
Conversation
164240c
to
fc2177e
Compare
|
||
[core_settings] | ||
log_level = "info" | ||
allow_root = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this be included? I don't think we should expect that root
account would be used by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why it's used here in the first place, I've copied the config file in the recent quickstart package.
one reason I can think of is that the quickstart package would be the first thing a user may use.
so we don't want to make any assumptions unless documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes much of a difference. Perhaps some people will want to run it with a root account, don't know how likely that is. We can either keep or remove this - the quickstart bundle isn't meant to be a secure deployment anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more worried that this quickstart config might be used as an example.
I'm OK with leaving it although I would prefer to remove it.
Fixes: parallaxsecond#534 Signed-off-by: Mohamed Omar Asaker <mohamed.omarasaker@arm.com>
fc2177e
to
8a10006
Compare
- Simplification, Use jq tool to extract the version instead of python - Fix commands used in the latest tag check - Remove redundant rm and mkdir commands - Correct the warning message - Build parsec-tool latest taged version Signed-off-by: Mohamed Omar Asaker <mohamed.omarasaker@arm.com>
8a10006
to
2b314d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change! Got a few comments, but perhaps some of them are just me misunderstanding stuff
@@ -0,0 +1,128 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend setting some flags for the script to make sure nothing fails silently:
set -euf -o pipefail
Signed-off-by: Mohamed Omar Asaker <mohamed.omarasaker@arm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 👍🏻
Fixes: #534
Signed-off-by: Mohamed Omar Asaker mohamed.omarasaker@arm.com