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: ensure docker image checks architecture #1589

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

mcwarman
Copy link
Member

Fix #1582

Moved away from setting remoteOptions for single architecture, as this was causing the multi conditions case to fail.

I can remove the tests if we don't want it to look up those images.

Test

To test this pull request, you can run the following commands:

cd pkg/plugins/resources/dockerimage
go test

Additional Information

Potential improvement

Mock the tests

@olblak
Copy link
Member

olblak commented Sep 12, 2023

Thanks for looking into this issue, my understanding is they are still images out there that uses the old schema so we may need to disable the architecture support if architecture/architectures is unset. like before your fix :p

@mcwarman
Copy link
Member Author

mcwarman commented Sep 12, 2023

I think the fix does potentially deal with that scenario, unless I've misunderstood what you are saying:

  • New default to be empty list of achitectures
  • on source if list is empty parse empty string to checkImages, else take the first in the list. source.go#L51-L60
  • on condtions if list is empty parse empty string to checkImages, else iterate over architectures condition.go#L26-L42
  • in checkImage then deals with empty arch by not checking it, if it is provided and its unsupported only return an error then.

@dduportal
Copy link
Contributor

I think the fix does potentially deal with that scenario, unless I've misunderstood what you are saying:

* New default to be empty list of achitectures

* on `source` if list is empty parse empty string to checkImages, else take the first in the list. [source.go#L51-L60](https://github.com/mcwarman/updatecli/blob/fix/docker-images-check-arch/pkg/plugins/resources/dockerimage/source.go#L51-L60)

* on `condtions` if list is empty parse empty string to checkImages, else iterate over architectures [condition.go#L26-L42](https://github.com/mcwarman/updatecli/blob/fix/docker-images-check-arch/pkg/plugins/resources/dockerimage/condition.go#L26-L42)

* in `checkImage` then deals with empty arch by not checking it, if it is provided and its unsupported only return an error then.
  
  * [main.go#L102-L104](https://github.com/mcwarman/updatecli/blob/fix/docker-images-check-arch/pkg/plugins/resources/dockerimage/main.go#L102-L104)
  * [main.go#L118-L136](https://github.com/mcwarman/updatecli/blob/fix/docker-images-check-arch/pkg/plugins/resources/dockerimage/main.go#L118-L136)

That looks a sane set of code paths 👍 Thanks for looking into it!

@olblak
Copy link
Member

olblak commented Sep 13, 2023

Thanks for the explanation.
I just did some testing, and I really like how you nicely solve the problem

Copy link
Member

@olblak olblak left a comment

Choose a reason for hiding this comment

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

@mcwarman Great PR

@olblak olblak merged commit cd2f873 into updatecli:main Sep 13, 2023
6 checks passed
@olblak olblak added the bug Something isn't working label Sep 13, 2023
@mcwarman mcwarman deleted the fix/docker-images-check-arch branch September 13, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docker Image] Condition does not honour the architecture filter
3 participants