Skip to content

ci: split perf workflow - #272

Merged
niemeyer merged 8 commits into
canonical:mainfrom
upils:fix/split-performance-worflow
Mar 30, 2026
Merged

ci: split perf workflow#272
niemeyer merged 8 commits into
canonical:mainfrom
upils:fix/split-performance-worflow

Conversation

@upils

@upils upils commented Mar 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator
  • Have you signed the CLA?

Do not expose the token used to write the comment to PRs from forks.
This also ensures that PRs open from external contributors will not trigger
the workflows.
In the workflow posting the comment, use a fine-grained token with the
minimum permissions (write PR and read Actions).

@upils
ci: split perf workflow
9ac5703 
Do not expose the token used to write the comment to PRs from forks.
@upils
upils requested review from cjdcordeiro and letFunny March 6, 2026 08:59
lczyk
lczyk reviewed Mar 6, 2026
View reviewed changes
Comment thread .github/workflows/performance.yaml Resolved
Comment thread .github/workflows/performance.yaml Resolved
letFunny
letFunny reviewed Mar 6, 2026
View reviewed changes

@letFunny letFunny left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A few comments, thank for looking into this Paul. It would be nice if we can get some guidance for security as well.

Comment thread .github/workflows/comment-pr.yaml Resolved
Comment thread .github/workflows/comment-perf.yaml Resolved
Comment thread .github/workflows/comment-pr.yaml Resolved
@lczyk lczyk mentioned this pull request Mar 6, 2026
Merged
3 tasks done
@upils
upils requested a review from cjdcordeiro March 9, 2026 10:31
@upils
upils requested a review from lczyk March 10, 2026 09:47
cjdcordeiro
cjdcordeiro reviewed Mar 10, 2026
View reviewed changes
Comment thread .github/workflows/comment-perf.yaml Resolved
letFunny
letFunny approved these changes Mar 10, 2026
View reviewed changes

@letFunny letFunny left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you Paul for taking care of this, it looks great. Let's see how it works once it is merged.

Comment thread .github/workflows/performance.yaml Resolved
@upils upils added the Bug An undesired feature ;-) label Mar 11, 2026
@lczyk

lczyk commented Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

nit, but you didn't tick 'Have you signed the CLA?' ;)

niemeyer
niemeyer requested changes Mar 16, 2026
View reviewed changes

@niemeyer niemeyer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My understanding is that this is addressing the running of workflows from PRs so that we don't leak unintended information or offer more access than necessary to the PR author. Yet, there's not a single comment being added about any of these situations, which means next week someone else can come here and take the chance in a different direction and the PR will likely go in as well.

Can we please be more clear about what this is addressing, how, and what shouldn't be done in that next PR? Please have the comments in places which are likely to make a difference on the follow ups.

@upils

upils commented Mar 16, 2026

Copy link
Copy Markdown
Collaborator Author

My understanding is that this is addressing the running of workflows from PRs so that we don't leak unintended information or offer more access than necessary to the PR author. Yet, there's not a single comment being added about any of these situations, which means next week someone else can come here and take the chance in a different direction and the PR will likely go in as well.

Can we please be more clear about what this is addressing, how, and what shouldn't be done in that next PR? Please have the comments in places which are likely to make a difference on the follow ups.

Your understanding is correct. I added a comment on each workflow, clarifying why they should stay separate and what to never do to protect secrets. See 38b27d9?

@upils upils mentioned this pull request Mar 20, 2026
Merged
1 task
niemeyer pushed a commit that referenced this pull request Mar 30, 2026
@upils
ci: pin all actions (#277)
4662d34 
Pin all actions used in workflows with hashes.
This also bumps setup-go and checkout actions version as I expect no breaking
change given how they are used.

Remaining actions not pinned in this PR are pinned or reworked in other PRs (see
#275, #274, #272)
niemeyer
niemeyer approved these changes Mar 30, 2026
View reviewed changes

@niemeyer niemeyer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks for the additional comments.

Comment thread .github/workflows/comment-perf.yaml
# The pull_request_target trigger MUST NOT be used as it bypasses the
# requirement of maintainer approval of a PR from an outside contributor and
# it is executed in the context of the target repository, giving access to
# secrets.

@niemeyer niemeyer Mar 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, this helps.

@niemeyer
niemeyer merged commit 5fb43b8 into canonical:main Mar 30, 2026
15 checks passed
@ROCKsBot

ROCKsBot commented Mar 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Command Mean [s] Min [s] Max [s] Relative
BASE 13.978 ± 0.123 13.831 14.221 1.00 ± 0.01
HEAD 13.911 ± 0.062 13.817 14.023 1.00

@niwamo

niwamo commented Mar 31, 2026

Copy link
Copy Markdown

Commenting here as Canonical's Security Operations Manager to acknowledge the contribution of Yarden Curiel in reporting the workflow vulnerability created by the former usage of pull_request_target. Your contributions are appreciated!

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

Reviewers

@niemeyer niemeyer niemeyer approved these changes
@cjdcordeiro cjdcordeiro cjdcordeiro approved these changes
+2 more reviewers
@lczyk lczyk lczyk approved these changes
@letFunny letFunny letFunny approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug An undesired feature ;-)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@upils @lczyk @ROCKsBot @niwamo @niemeyer @cjdcordeiro @letFunny