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

Adding breaking change label to container #1809

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Adding breaking change label to container #1809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ddad4b2
adding breaking change label to container
javanlacerda Sep 20, 2024
6939ae7
create variable for FORMATED_LABEL
javanlacerda Oct 8, 2024
878bd57
adding read permission for PRs and adding full tag
javanlacerda Oct 8, 2024
1197ca9
fix full tag
javanlacerda Oct 8, 2024
358a756
adding github token for github cli usage
javanlacerda Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion .github/workflows/container-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ jobs:
permissions:
id-token: write
contents: read
pull-requests: read

steps:
- uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
Expand Down Expand Up @@ -59,5 +60,23 @@ jobs:
- name: creds
run: gcloud auth configure-docker --quiet

- name: Formatted labels
id: labels
env:
GH_TOKEN: ${{ github.token }}
run: |
FORMATED_LABELS="--image-label commit-hash=$GITHUB_SHA"

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need commit hash as a label? Looks like we include that as a tag already.

Copy link
Member

Choose a reason for hiding this comment

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

if that does not hurt, it is an extra thing :)

BRANCH_NUMBER=$(gh pr list --state all --search "sha:$GITHUB_SHA" --label "breaking-change" | awk '{print $1}')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check if we need to create a PAT for this or if the default token is fine? We sometimes hit rate limits when querying GitHub. I’d rather not manage another token but just wanna check.

Cc @cpanato

Copy link
Contributor

Choose a reason for hiding this comment

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

One other idea is to change this to fire on PR closure, like what’s documented in https://stackoverflow.com/questions/71523153/github-actions-on-pull-request-merged-with-specific-labels

You could then checkout from HEAD based on the git sha.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, that might not work - commit shas will differ after being squashed to merge to HEAD.

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 think it should be fine as I tested this same action in my fork and it worked properly. https://github.com/javanlacerda/fulcio/actions/runs/10997017464/job/30531560844. Just hadn't the authorization to push the image, what is expected.

Copy link
Member

Choose a reason for hiding this comment

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

i see this and will reply tomorrow (25/sept/2024) sorry !

Copy link
Member

Choose a reason for hiding this comment

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

We sometimes hit rate limits when querying GitHub

gh pr list does not use the "search" endpoint which I believe is the problematic one (since search rate limit is 30/minute instead of 5000/hour like others so temporary spikes can easily hit it) so I guess this is fine.

Copy link
Member

Choose a reason for hiding this comment

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

if i run that locally it does not work, need to do a setup before

$ gh pr list --state all --search "sha:7920be28f7f58020c4da8ab9bb78554ad7cd67ea" --label "breaking-change"
X No default remote repository has been set for this directory.

please run `gh repo set-default` to select a default remote repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, that's weird because it works on my fork. Maybe because we do the checkout at line 37?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

example here https://github.com/javanlacerda/fulcio/actions/runs/11242081243/job/31254992474

Copy link
Member

Choose a reason for hiding this comment

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

ok, so maybe is a local thing, not related to the gh actions env. thanks for the clarification

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about how this will be used in PGI: Have you thought about how we will handle if multiple PRs are merged in a short period with only one labeled as a breaking change? Are we going to search back through all containers built after the last deployment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! The project that will deploy it will be responsible for detecting it.

echo "Branch Number: $BRANCH_NUMBER"

# Check if a pull request number was found
if [ -n "$BRANCH_NUMBER" ]; then
FORMATED_LABELS+=" --image-label breaking-change=true"
fi
echo "FORMATED_LABELS='$FORMATED_LABELS'" >> $GITHUB_OUTPUT
cpanato marked this conversation as resolved.
Show resolved Hide resolved

- name: container
run: KO_PREFIX=gcr.io/projectsigstore/fulcio/ci/fulcio make sign-keyless-ci
run: |
echo "Formated Label: ${{ steps.labels.outputs.FORMATED_LABELS }}"
javanlacerda marked this conversation as resolved.
Show resolved Hide resolved
KO_PREFIX=gcr.io/projectsigstore/fulcio/ci/fulcio FORMATED_LABEL=${{ steps.labels.outputs.FORMATED_LABELS }} make sign-keyless-ci
11 changes: 9 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ GHCR_PREFIX ?= ghcr.io/sigstore

FULCIO_YAML ?= fulcio-$(GIT_TAG).yaml

# It should be blank for default builds
FORMATED_LABEL ?=

GITHUB_RUN_NUMBER ?= "local"

FULL_TAG := "0.$(shell date +%Y%m%d).$(GITHUB_RUN_NUMBER)-ref.$(GIT_VERSION)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this include git sha or git version?

Copy link
Member

Choose a reason for hiding this comment

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

this when we release will be used, not sure if we want that format


# Binaries
PROTOC-GEN-GO := $(TOOLS_BIN_DIR)/protoc-gen-go
PROTOC-GEN-GO-GRPC := $(TOOLS_BIN_DIR)/protoc-gen-go-grpc
Expand Down Expand Up @@ -122,8 +129,8 @@ $(PROTOC-API-LINTER): $(TOOLS_DIR)/go.mod
ko:
# fulcio
LDFLAGS="$(LDFLAGS)" GIT_HASH=$(GIT_HASH) GIT_VERSION=$(GIT_VERSION) \
KO_DOCKER_REPO=$(KO_PREFIX)/fulcio ko resolve --bare \
--platform=linux/amd64 --tags $(GIT_VERSION) --tags $(GIT_HASH) \
KO_DOCKER_REPO=$(KO_PREFIX)/fulcio ko resolve $(FORMATED_LABEL) --bare \
javanlacerda marked this conversation as resolved.
Show resolved Hide resolved
--platform=linux/amd64 --tags $(GIT_VERSION) --tags $(GIT_HASH) --tags $(FULL_TAG) \
--image-refs fulcioImagerefs --filename config/ > $(FULCIO_YAML)

.PHONY: ko-local
Expand Down
Loading