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

Porter triggers autobuild when detecting that invocation image does not exist #1828

Conversation

joshuabezaleel
Copy link
Contributor

What does this change

Check whether the invocation image exists in the host registry and trigger autobuild of the image by setting the return value of IsBundleUpToDate to false.

What issue does it fix

Closes #1756

Checklist

  • Unit Tests
  • Documentation
  • Schema (porter.yaml)

@joshuabezaleel joshuabezaleel force-pushed the autobuild-invocation-image-not-exist branch from cda0121 to dc5e6c9 Compare November 17, 2021 11:33
@joshuabezaleel
Copy link
Contributor Author

joshuabezaleel commented Nov 17, 2021

Hi @carolynvs ! 😄
This is my attempt to tackle the #1756 issue, might not be the best one we should be pursuing with so looking forward to the review!

So basically I added the method to check whether an invocation image exists on the RegistryProvider since the docker client is only available on the RegistryProvider and use that particular method to set the upToDate value to false which will trigger the build process.

Along with this comment, I attached the video demo of it. The porter of v0.38.7 is the version that I downloaded on the release page while the v1.0.0-alpha.5-4-gca49edf8 (ca49edf8) is from the one that I build locally from source.

porter-autobuild-invocation-image-newest.mp4

@carolynvs carolynvs self-assigned this Nov 22, 2021
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

This is really close. Most of my comments are about how to get the code a bit closer to fitting in with the rest of porter's conventions. Since you are interested in eventually being a maintainer, I wanted to give you more information about how we have been doing things.

pkg/cnab/cnab-to-oci/provider.go Outdated Show resolved Hide resolved
pkg/cnab/cnab-to-oci/registry.go Outdated Show resolved Hide resolved
pkg/cnab/cnab-to-oci/registry.go Outdated Show resolved Hide resolved
pkg/porter/stamp.go Outdated Show resolved Hide resolved
…along with some explanation comment.

2. Use invocationImage directly as an input to list images
3. Wrap error from Docker SDK and remove error wrap on stamp.go
4. Fix wording to "local image cache" from "host registry"

Signed-off-by: joshuabezaleel <[email protected]>
@joshuabezaleel joshuabezaleel force-pushed the autobuild-invocation-image-not-exist branch from 98a1145 to 3b48c78 Compare January 10, 2022 16:48
@joshuabezaleel
Copy link
Contributor Author

The workflow should still work just fine as the attached video below after the recent commit even though I'm not sure why did it take so long to build the image, perhaps it was just my machine or the internet connection.
https://user-images.githubusercontent.com/7043511/148804851-f06b0bd5-ba42-4015-9cc4-6aa420474471.mp4

Since I'm new to the open source world I have to be honest that I often feel anxious as to making beginner mistakes here and there and will burden the team/maintainer but reading your code review has always been a delight as it has always been so thoughtful. Thank you lots, @carolynvs ! 😄

@carolynvs
Copy link
Member

/azp run porter-integration

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this bug! 💖

@carolynvs carolynvs merged commit 76f07b6 into getporter:release/v1 Jan 11, 2022
joshuabezaleel added a commit to joshuabezaleel/porter that referenced this pull request Feb 8, 2022
…ot exist (getporter#1828)

* Porter triggers autobuild when detecting that invocation image does not exist

Signed-off-by: joshuabezaleel <[email protected]>

* 1. Change function name from IsInvocationImagExists to IsImageCached along with some explanation comment.
2. Use invocationImage directly as an input to list images
3. Wrap error from Docker SDK and remove error wrap on stamp.go
4. Fix wording to "local image cache" from "host registry"

Signed-off-by: joshuabezaleel <[email protected]>
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