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

Name and dockerfile as required inputs. #12161

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Name and dockerfile as required inputs. #12161

wants to merge 4 commits into from

Conversation

dekiel
Copy link
Contributor

@dekiel dekiel commented Oct 17, 2024

Description

Changes proposed in this pull request:

Name and dockerfile path of image to build must be provided in a call to image-builder.
These inputs must not be optional.
Using some default values for these parameters doesn't make sense. User must specify which image is build.

@dekiel
Name and dockerfile path of image to build must be provided in a call…
8d96897 
… to image-builder. These inputs must not be optional.
@dekiel dekiel requested review from neighbors-dev-bot and a team as code owners October 17, 2024 13:43
@kyma-bot kyma-bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 17, 2024
@dekiel dekiel added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed cla: yes Indicates the PR's author has signed the CLA. labels Oct 17, 2024
@kyma-bot kyma-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 17, 2024
@dekiel dekiel added image-builder and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 17, 2024
@kyma-bot kyma-bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 17, 2024
Sawthis
Sawthis previously approved these changes Oct 17, 2024
View reviewed changes
@kyma-bot kyma-bot added lgtm Looks good to me! no-changes labels Oct 17, 2024
@kyma-bot
Copy link
Contributor

kyma-bot commented Oct 17, 2024 •
edited
Loading

Plan Result

CI link

No changes. Your infrastructure matches the configuration.

@dekiel
Copy link
Contributor Author

dekiel commented Oct 18, 2024

I will notify teams we plan this breaking change. I will give them two weeks to review their workflows and adjust to the change. For that time moving PR to blocked/waiting.

@dekiel dekiel self-assigned this Oct 21, 2024
@Sawthis Sawthis assigned dekiel and unassigned dekiel and Sawthis Nov 6, 2024
@k15r
Copy link
Contributor

k15r commented Nov 6, 2024

why not use some different defaults instead of requiring the name and Dockerfile-name?
if you use docker on your terminal all you have to specify is the PATH. so docker build . is a valid way to build your image. I agree the resulting image name:tag is barely usable, but the name could be by default derived from the repository.
Building from the root of the repository and using a file called Dockerfile would not be to unreasonable.

@dekiel
Copy link
Contributor Author

dekiel commented Nov 6, 2024

@k15r such defaults sounds good.
image name: repository name
dockerfile path: ./Dockerfile

@dekiel
Merge remote-tracking branch 'upstream/main' into no-defaults-in-imag…
3865a46 
…e-builder

# Conflicts:
#	.github/workflows/image-builder.yml
@dekiel
Resolve conflicts.
120503e 
./Dockerfile as default dockerfile path.
@kyma-bot kyma-bot removed the lgtm Looks good to me! label Nov 6, 2024
@kyma-bot
Copy link
Contributor

kyma-bot commented Nov 6, 2024

New changes are detected. LGTM label has been removed.

@kyma-bot kyma-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 6, 2024
@KacperMalachowski
Copy link
Contributor

I need to verify if it will work for every event supported by image-builder.

@KacperMalachowski
Copy link
Contributor

Looks good, for details you can check: https://docs.github.com/en/webhooks/webhook-events-and-payloads

@Sawthis
Copy link
Contributor

Sawthis commented Nov 6, 2024

/hold documentation needs to be updated

Sawthis
Sawthis requested changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@Sawthis Sawthis left a comment

Choose a reason for hiding this comment

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

Looks good to me.

We should wait with merge of this PR until 20th of November as was communicated on Kyma PO meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sawthis Sawthis Sawthis requested changes

@neighbors-dev-bot neighbors-dev-bot Awaiting requested review from neighbors-dev-bot neighbors-dev-bot is a code owner

@KacperMalachowski KacperMalachowski Awaiting requested review from KacperMalachowski KacperMalachowski is a code owner automatically assigned from kyma-project/prow

@akiioto akiioto Awaiting requested review from akiioto

@szumejker szumejker Awaiting requested review from szumejker

Requested changes must be addressed to merge this pull request.

Assignees

@dekiel dekiel

@Sawthis Sawthis

Labels
cla: yes Indicates the PR's author has signed the CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. image-builder no-changes size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dekiel @kyma-bot @k15r @KacperMalachowski @Sawthis