-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update latest version checking to avoid warning in dev mode #37
Conversation
if not version == arlas_cli_version: | ||
latest_version = json.get("info", {}).get("version", None) | ||
if latest_version: | ||
if arlas_cli_version == "_".join(["arlas", "cli", "versions"]): |
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.
if arlas_cli_version == "arlas_cli_versions":
is good enough.
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 worried that it would be changed by the pattern replacement in the file during the release script, which would mean that this condition is always satisfied. It is just one way to create the same string, it can be an other such as "arlas"+"_cli_versions"
if you think it looks clearer
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.
Ok but I do not see how this helps to prevent it?
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.
and by the way, if we change it, the first thing we will do is a full search in the project on "arlas_cli_versions" and with your solutions we will miss it and therefore the problem you want to avoid will appear.
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.
As I understand, the sed -i.bak 's/arlas_cli_versions/'${VERSION}'/' target/src/arlas/cli/cli.py
replace the arlas_cli_versions
in the cli.py file. With the construction I propose, the value is not replaced and I can check if the arlas_cli_version variable still contains "arlas_cli_versions" -which is a local dev usage- or not
(I noticed you added a 's' to avoid replacing the variable itself)
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.
Ok I understand.
NB In dev mode, the
arlas_cli_versions
string has not been replaced by the real arlas cli version (it appends during the release script)