-
Notifications
You must be signed in to change notification settings - Fork 95
builder-manifest: Override branch with --default-branch when specified #696
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
Conversation
| @@ -0,0 +1,26 @@ | |||
| { | |||
| "id": "org.test.DefaultBranch", | |||
| "runtime": "org.gnome.Platform", | |||
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.
| "runtime": "org.gnome.Platform", | |
| "runtime": "org.test.Platform", |
Didn't want to bother figuring out where the test runtime was
|
I also want to write proper tests for this before we merge it. The scenarios we need to test are:
|
|
lol I never wrote the commit subject |
Commit 00f63cf added default-branch with the stated intention of being able to override the branch specified in the manifest. The name is a bit confusing. However the scenario tested was only when there was no specified branch at all, and thus the behavior was a bit buggy. Till now, if you had both specified "branch": "something" in the manifest and used --default-branch=test, the end result in the ostree repo would be for flatpak-builder to use the branch name specified in the manifest. ``` $ flatpak-builder --default-branch=test --force-clean --repo=_build/repo/ app/ tests/test-default-branch.json $ ostree refs --repo=_build/repo/ app/org.test.DefaultBranch/x86_64/something ``` With this change, we properly pick up default-branch if its set, since it can only be set through the cli and its the explicit intention that we want to override it. ``` $ meson devenv -C _build/ flatpak-builder --default-branch=test --user --force-clean --repo=repo_test/ app/ ../tests/test-default-branch.json $ ostree refs --repo=_build/repo_test app/org.test.DefaultBranch/x86_64/test ```
b236e3d to
8e94311
Compare
| return self->runtime_version ? self->runtime_version : "master"; | ||
| } | ||
|
|
||
| /* default-branch can only be set through the cli, where branch |
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.
default-branch can be set through manifest though.
|
It was intentional that the I don't agree with the complicated design; so my plan was to remove the But I guess making both being overridden by the arg is ok. In any case this is a breaking change and has to go through unstable branch first. This also needs coordination at least on Flathub before merging as there are a number of extensions deriving their expected branch from the |
This behavior is documented in docs so you need to update the docs while changing it: flatpak-builder/doc/flatpak-manifest.xml Lines 62 to 71 in 4e23835
|
|
Indeed, re-reading the manifest it actually works as expected.
Where this goes wrong imo is the following then.
Let's close this PR and instead go fix the manifests then I guess |
builder-manifest: Override branch with --default-branch when specified
Commit 00f63cf added default-branch with the stated intention of being
able to override the branch specified in the manifest. The name is a bit
confusing.
However the scenario tested was only when there was no
specified branch at all, and thus the behavior was a bit buggy.
Till now, if you had both specified "branch": "something" in the
manifest and used --default-branch=test, the end result in the
ostree repo would be for flatpak-builder to use the branch name
specified in the manifest.
With this change, we properly pick up default-branch if its set,
since it can only be set through the cli and its the explicit intention
that we want to override it.