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

Add Image builder should_build method for more customizable build behavior #2458

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

cosmicBboy
Copy link
Contributor

Why are the changes needed?

Prior to this PR, the build behavior is determined by the ImageBuildEngine. This makes the messages confusing on e.g. with the serverless UCImageBuilder, which has different logic that determines whether an image should be built or not. The CLI messages say the images doesn't exist even when it does.

What changes were proposed in this pull request?

This change introduces a should_build method to the ImageSpecBuilder base class. This method should take an image spec and output True if the image should be built or False if the image already exists.

How was this patch tested?

This PR was manually tested on a Union tenant, unit tests are pending

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

…mizeable by builder plugin

Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
pingsutw added 2 commits June 5, 2024 13:24
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw changed the title [wip] Add Image builder should_build method for more customizable build behavior Add Image builder should_build method for more customizable build behavior Jun 5, 2024
Copy link
Contributor Author

@cosmicBboy cosmicBboy left a comment

Choose a reason for hiding this comment

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

This looks good to me! @katrina mind a taking a look? context: https://unionai.slack.com/archives/C05EHTFEFB9/p1717455305121219

@cosmicBboy cosmicBboy merged commit 3555f2e into master Jun 6, 2024
46 checks passed
bgedik pushed a commit to bgedik/flytekit that referenced this pull request Jul 3, 2024
…ehavior (flyteorg#2458)

* add ImageSpecBuilder.should_build method to make building logic customizeable by builder plugin

Signed-off-by: Niels Bantilan <[email protected]>

* updates

Signed-off-by: Niels Bantilan <[email protected]>

* ping's update

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

---------

Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: bugra.gedik <[email protected]>
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
…ehavior (#2458)

* add ImageSpecBuilder.should_build method to make building logic customizeable by builder plugin

Signed-off-by: Niels Bantilan <[email protected]>

* updates

Signed-off-by: Niels Bantilan <[email protected]>

* ping's update

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

---------

Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
@cosmicBboy cosmicBboy deleted the image-builder-should-build branch August 20, 2024 14:57
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.

3 participants