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.

Sign up for GitHub

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

Jump to bottom

feat(core): Support S2I for builder image generation #4417

Merged
merged 2 commits into from
May 29, 2023
Merged

feat(core): Support S2I for builder image generation #4417

merged 2 commits into from
May 29, 2023

Conversation

gansheer
Copy link
Contributor

Ref #4297

Motivation

Add S2I support for builder image generation for better support on Openshift.

Description

  • Add initialize builder image on catalog:
  • creates (or replace) a BuildConfig resource
  • creates (or replace) an ImageStream resource
  • create a new builder image from dockerfile and source binary
  • Light refactoring of S2I code

The HOME env variable has been added on builder image execution to some errors on file permissions without.

Release Note

Add S2I support for builder image generation

@gansheer
Copy link
Contributor Author

gansheer commented May 26, 2023 •
edited
Loading

@lburgazzoli @astefanutti and anyone who have some knowledge about Openshift please don't hesitate to review, I would really like some openshift feedback 😄

squakez
squakez reviewed May 26, 2023
View reviewed changes
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

LGTM. There is some code duplication with the builder/s2i. As we want to release CK 2 ASAP, consider if you want to address in this PR or create a follow up issue not to left pending any tech debt unattended. Thanks!

@gansheer gansheer force-pushed the feature/4297_s2i_builder_image branch from 81c4d80 to 5a7b178 Compare May 26, 2023 14:13
@gansheer
feat(core): Support S2I for builder image generation
a4a40ad 
* Add initialize builder image on catalog with imagestream and buildconfig resource who's owner is the CamelCatalog
* Light refactoring of S2I code

Ref apache#4297
@gansheer gansheer force-pushed the feature/4297_s2i_builder_image branch from 5a7b178 to a4a40ad Compare May 26, 2023 14:23
@squakez
Copy link
Contributor

squakez commented May 29, 2023

@gansheer please, let me know when it's ready to merge. If you're addressing the comments provided in the PR or we create follow up issues.

@gansheer
Copy link
Contributor Author

gansheer commented May 29, 2023 •
edited
Loading

@gansheer please, let me know when it's ready to merge. If you're addressing the comments provided in the PR or we create follow up issues.

On my side it is ready to merge

I took care of some part of the refactoring, but for the more complex part of refactoring I would prefer we open a follow up issue to be dealt with when the olm tests are more reliable after the 1.12.1 release as it could be impacted by #4418.

@squakez squakez merged commit bac85e1 into apache:main May 29, 2023
@squakez
Copy link
Contributor

squakez commented May 29, 2023

Merged. Please, take care to log any relevant follow up issue.

@gansheer gansheer mentioned this pull request May 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squakez squakez squakez left review comments

@oscerd oscerd oscerd approved these changes

@claudio4j claudio4j claudio4j approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gansheer @squakez @claudio4j @oscerd