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

💝 Auto-skip push to registry if secrets are not set #12

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

Conversation

cardil
Copy link
Member

@cardil cardil commented Dec 7, 2022

Changes

  • 💝 Auto-skip push to registry, if secrets are not set
    • Allows to build images, checking the build

/kind enhancement

@openshift-ci
Copy link

openshift-ci bot commented Dec 7, 2022

@cardil: The label(s) kind/enhancement cannot be applied, because the repository doesn't have them.

In response to this:

Changes

  • 💝 Auto-skip push to registry, if secrets are not set
    • Allows to build images, checking the build

/kind enhancement

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot requested review from lberk and skonto December 7, 2022 13:31
@openshift-ci
Copy link

openshift-ci bot commented Dec 7, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cardil
Once this PR has been reviewed and has the lgtm label, please assign skonto for approval by writing /assign @skonto in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cardil cardil added the kind/enhancement New feature or request label Dec 7, 2022
@cardil
Copy link
Member Author

cardil commented Dec 7, 2022

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

I lack context, what's the reason for this?

@cardil
Copy link
Member Author

cardil commented Dec 7, 2022

I would like to have option to run those builds for PRs, and other development branches, so you may know that it should build the images well, before the merge.

@pierDipi
Copy link
Member

pierDipi commented Dec 7, 2022

I would like to have option to run those builds for PRs, and other development branches, so you may know that it should build the images well, before the merge.

In that case, can't we use something like if: github.event_name == 'push'?
it seems like an hack to use secrets presence to infer push or not push

@cardil
Copy link
Member Author

cardil commented Dec 7, 2022

This is a reusable action that builds and pushes images.

It makes sense to push only if the secrets are set, right? Depending on secrets makes far more sense than limiting this to work in pushes only, which for me, is unrelated to pushing to the registry.

@mgencur
Copy link
Contributor

mgencur commented Dec 8, 2022

What does it bring ... depending on the secrets? This github action will always run on github, not locally.
It's more understandable to have what @pierDipi suggested. The user knows that pushing will be done only on push, not in pre-submit phase. If you want this for debugging purposes to run this on your repo then pushing the images will fail anyway if you don't have the secrets set, which might be a good signal. In other words, failing the github action if the secrets are not provided is expected.
Depending on the presence of secrets only raises questions like the one from @pierDipi . Using if: github.event_name == 'push' makes it clear immediately, it is self-documenting.

@cardil
Copy link
Member Author

cardil commented Dec 8, 2022

@mgencur wrote:

What does it bring ... depending on the secrets? This github action will always run on github, not locally.

Sure, it will always run on Github. I thought this may allow testing the build on fork, as the forks doesn't have secrets configured. You push your code to your fork, and Github action fires on your fork repo, before you even create a PR.

It's more understandable to have what @pierDipi suggested. The user knows that pushing will be done only on push, not in pre-submit phase. If you want this for debugging purposes to run this on your repo then pushing the images will fail anyway if you don't have the secrets set, which might be a good signal. In other words, failing the github action if the secrets are not provided is expected. Depending on the presence of secrets only raises questions like the one from @pierDipi . Using if: github.event_name == 'push' makes it clear immediately, it is self-documenting.

I think both of those conditions are valid. I'll add github.event_name == 'push'. That would allow to run this workflow for the pull_request event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants