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

Adding check_push_rights #1

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

erikbosch
Copy link
Contributor

Currently this workflow exists in both kuksa.val and kuksa.val.feeders. As we are to split repos we might get even more, and then it is good to have a common reusable location for it

Related to:

eclipse/kuksa.val#680
eclipse-kuksa/kuksa.val.feeders#145

Those two PRs needs to be updated when this PR is merged.
We can also discuss release pattern for this repo, do we want to tag after every merged (significant) PR, or just when major backward incompatible changes are introduced. In this case, for now, users could easily use main as version identifier so no urgent need to tag.

(("${{ github.event.pull_request.head.repo.full_name }}" == "eclipse/kuksa.val") ||\
("${{ github.event.pull_request.head.repo.full_name }}" == "eclipse/kuksa.val.feeders") || \
("${{ github.event.pull_request.head.repo.owner.login }}" == "eclipse-kuksa")) ]] ; then
echo "We are an internal pull request , so we should have rights"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know if any build tries to push to ghcr for pull requests (if we get true here). We could argue that it is not good practice - we can as well use ttl.sh for that purpose. In some (all?) builds we already have an extra check like:

if: ${{ needs.checkrights.outputs.have_secrets == 'true' && github.event_name != 'pull_request' }}

But if we change - should we then possibly rename it so it rather returns push_ghcr=true and call it check_ghcr_push. That would simplify usage and would make it easier for us to change behavior for all kuksa repos using this workflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. So we do not push any PRs to GHCR anymore, and it is also not really recommended I would say, as there are some quotas in play. So this action really decides "Should we push to GHCR?" depending on whether we have the rights and whether we want to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can refactor it and try it out

@SebastianSchildt
Copy link
Contributor

I would say, we should just use tagged version and always tag, when something "significant/incompatible" changes in any of the actions hosted here. If just fixing a bug in one, it might be ok, to just retag.

In any case the only disadvantage is, as any change in action demands a new tag, numbers might go up fast. But it will be quite robust, we can change "everything" here, without disturbing workflows

@erikbosch
Copy link
Contributor Author

I would say, we should just use tagged version and always tag, when something "significant/incompatible" changes in any of the actions hosted here. If just fixing a bug in one, it might be ok, to just retag.

In any case the only disadvantage is, as any change in action demands a new tag, numbers might go up fast. But it will be quite robust, we can change "everything" here, without disturbing workflows

We could actually use a "double tag" mechanism, some other actions use that. Like have both a major tag X and a minor X.Y and only increase major in case of backward incompatible changes. Then users can specify minor version if they want to stick to a specific version, but major if they accept changes that does not affect backward compatibility

  • Add a tag 2.0 to current tag 2
  • When this is merged move tag 2 to the new commit and also add a tag 2.1

That way users can decide if they just want the lates

@erikbosch
Copy link
Contributor Author

PR refactored based on discussion. Seems to work as expected for kuksa.val and kuksa.val.feeders. @SebastianSchildt -feel free review again. Suggested way forward:

  • This PR is merged (unless there are comments)
  • Other PRs updated to use eclipse rather than erikbosch
  • If it works I suggest that we use "dual versions", i.e. tag old commit as 2.0, and move tag 2 to this commit and also tag the new commit as 2.1, then update the other PRs to use version 2

Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

The suggested approach to tagging sounds reasonable, let's try it.

Also this PR "looks good", and anyway merging will not break anything, that might only happen, once we start to use this. So lgtm

@SebastianSchildt SebastianSchildt merged commit c781b47 into eclipse-kuksa:main Oct 4, 2023
1 check passed
@erikbosch erikbosch deleted the check_push branch October 31, 2024 13:15
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