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

fix: viur build release now sticks to versions defined in project.json #191

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Grashalmbeisser
Copy link
Collaborator

No description provided.

@phorward phorward changed the title fix: Build Release now installs version from project.json fix: viur build release now sticks to versions defined in project.json Dec 18, 2024
Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

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

Hello @Grashalmbeisser, please rethink this process entirely. The version number should be sticking when there is a valid definition provided.

Why not allow for

"admin": {
    "command": "viur package install admin",
    "kind": "exec",
    "version": "4.6.6"
}

to just stick to 4.6.6 and

"admin": {
    "command": "viur package install admin",
    "kind": "exec",
    "version": "LATEST"
}

to install latest version?

This is now another possible solution for this problem. Important is, that only "viur package install"-commands are being processed this way and that its clear which version number should be used.

Comment on lines 58 to 61
if "." in build_cfg["command"]:
utils.system(build_cfg["command"])
else:
utils.system(build_cfg["command"] + " " + build_cfg["version"])
Copy link
Member

Choose a reason for hiding this comment

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

Any "exec"-build command with a "." in its command string is now assumed to be a version number? And what if build_cfg["version"] does not exist?

In one of my projects, I have this definition, which immediately triggers this code-path as it contains a ".", an will fail as there is no "version" key:

"editor": {
    "clean": "rm -rf ./deploy/editor",
    "command": "python sources/editor/flare/tools/flare.py -s sources/editor -t deploy/editor",
     "kind": "exec"
}

Please rethink this part entirely. use a regular expression to determine if this is an admin install-command like viur package install admin with a valid version number behind. You can extract the version number using regular expression as well.

@@ -8,6 +8,7 @@
from pathlib import Path
from urllib.request import urlretrieve
from . import cli, echo_error, echo_info
from operator import itemgetter
Copy link
Member

Choose a reason for hiding this comment

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

???

@phorward phorward linked an issue Dec 19, 2024 that may be closed by this pull request
@phorward
Copy link
Member

This is now another possible solution for this problem. Important is, that only "viur package install"-commands are being processed this way and that its clear which version number should be used.

I'm still fan of my proposal provided in #188 (comment), why are packages and exec build commands not separated, especially because there currently is only one package called "admin" for future projects, as "scriptor" is abandoned and "vi" is also dead.

@phorward
Copy link
Member

@Grashalmbeisser the easiest solution would be to remove the "version"-key from the exec-definition, and just maintain the "command". Either it is "viur package install admin" to have the latest version, or "viur package install admin 4.6.6" to stick to a specific version. Then you can just remove this current decision making process etc. and JUST rely on this little information as the single source of truth.

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.

'viur build release' only updates to latest Version
2 participants