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

targets: Handle OrigUri and OrigUriApps when adding targets #442

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

detsch
Copy link
Member

@detsch detsch commented Nov 14, 2024

For custom apps targets, origUri must be set, while uri and origUriApps must be empty.
For custom ostree targets, origUriApps must be set, while uri and origUri must be empty.


@mike-sul this change seems to work, but is not enough. Apps targets generated by our CI are keeping the origUriApps field set. I'd say we need to fix the behavior there before merging this change to fioctl. Otherwise, an old origUriApps value is carried on indefinitely.

@detsch detsch requested a review from mike-sul November 14, 2024 18:43
For custom apps targets, origUri must be set, while uri and origUriApps
must be empty.
For custom ostree targets, origUriApps must be set, while uri and
origUri must be empty.

Signed-off-by: Andre Detsch <andre.detsch@foundries.io>
@detsch detsch force-pushed the detsch-fix-orig-uri branch from ba0553b to 363f6a8 Compare November 14, 2024 18:46
@mike-sul
Copy link
Contributor

mike-sul commented Nov 15, 2024

Apps targets generated by our CI are keeping the origUriApps field set.

If by "CI" do you mean the publish job/run, then setting both uri and origUriApps to the same value (URI to the given publish run) is probably fine.

If origUriApps refers to one of the previous publish runs then this is the issue, please create a ticket and assign to me.

@detsch
Copy link
Member Author

detsch commented Nov 17, 2024

Once foundriesio/ci-scripts#363 is merged, I'll test this patch a little bit more, to make sure there is no additional change required.

@mike-sul mike-sul marked this pull request as ready for review November 18, 2024 09:18
@mike-sul
Copy link
Contributor

Once foundriesio/ci-scripts#363 is merged, I'll test this patch a little bit more, to make sure there is no additional change required.

The ci-script has been merged.

@detsch
Copy link
Member Author

detsch commented Nov 18, 2024

Did some additional testing, looks OK.

@detsch detsch merged commit 3620f42 into foundriesio:main Nov 18, 2024
8 checks passed
@detsch detsch deleted the detsch-fix-orig-uri branch November 18, 2024 18:00
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.

2 participants