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

feat: adds proper structure to automatic bumping PRs #3469

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

simzzz
Copy link
Contributor

@simzzz simzzz commented Feb 11, 2025

Description:
Snyk PR titles now automatically add proper conventional type prefixes and appropriate labels.

Related issue(s):

Fixes #3319

Notes for reviewer:
Judging from the closed PR's by Snyk, I have made the assumption that they would always start with [Snyk] in their title and that they would all be a version upgrade or a security upgrade.

Checklist

@simzzz
Copy link
Contributor Author

simzzz commented Feb 11, 2025

@quiet-node I noticed you add your details when signing off the commit message of Snyk's PR's so I included them to the YAML file. We can further discuss if that's appropriate and think of alternatives if necessary

Copy link

github-actions bot commented Feb 11, 2025

Test Results

 19 files   -   8  274 suites   - 116   37m 29s ⏱️ - 38m 53s
615 tests  -   8  608 ✅ + 12  4 💤 ±0  3 ❌  - 20 
667 runs   - 468  660 ✅  - 439  4 💤  - 4  3 ❌  - 25 

For more details on these failures, see this check.

Results for commit 51cbf7b. ± Comparison against base commit 3e057b5.

This pull request removes 8 tests.
"after all" hook in "RPC Server Acceptance Tests" ‑ RPC Server Acceptance Tests "after all" hook in "RPC Server Acceptance Tests"
"before all" hook for "emits an approval event" ‑ RPC Server Acceptance Tests Acceptance tests @erc20 Acceptance Tests HTS token should behave like erc20 transfer from when the token owner is not the zero address when the recipient is not the zero address when the spender has enough allowance "before all" hook for "emits an approval event"
"before all" hook for "reverts" ‑ RPC Server Acceptance Tests Acceptance tests @erc20 Acceptance Tests HTS token should behave like erc20 transfer from when the token owner is not the zero address when the recipient is not the zero address when the spender does not have enough allowance when the token owner has enough balance "before all" hook for "reverts"
"before all" hook for "should execute "eth_getCode" for hts token" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode "before all" hook for "should execute "eth_getCode" for hts token"
"before all" hook in "@api-batch-2 RPC Server Acceptance Tests" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before all" hook in "@api-batch-2 RPC Server Acceptance Tests"
"before all" hook in "Debug API Test Suite" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests Debug API Test Suite "before all" hook in "Debug API Test Suite"
"before each" hook for "reverts" ‑ RPC Server Acceptance Tests Acceptance tests @erc20 Acceptance Tests HTS token should behave like erc20 transfer from when the token owner is not the zero address when the recipient is not the zero address when the spender does not have enough allowance "before each" hook for "reverts"
"before each" hook for "should execute "eth_getStorageAt" request against an inactive address (contains no data) and receive a 32-byte-zero-hex string " ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before each" hook for "should execute "eth_getStorageAt" request against an inactive address (contains no data) and receive a 32-byte-zero-hex string "

♻️ This comment has been updated with latest results.

@simzzz simzzz self-assigned this Feb 11, 2025
@simzzz simzzz added the internal For changes that affect the project's internal workings but not its outward-facing functionality. label Feb 11, 2025
@simzzz simzzz added this to the 0.67.0 milestone Feb 11, 2025
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.62%. Comparing base (3e057b5) to head (51cbf7b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3469      +/-   ##
==========================================
- Coverage   86.42%   85.62%   -0.80%     
==========================================
  Files          65       69       +4     
  Lines        4552     4731     +179     
  Branches      979      998      +19     
==========================================
+ Hits         3934     4051     +117     
- Misses        316      396      +80     
+ Partials      302      284      -18     
Flag Coverage Δ
config-service 95.16% <ø> (ø)
relay 79.55% <ø> (ø)
server 83.60% <ø> (ø)
ws-server 36.31% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 9 files with indirect coverage changes

Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

Thanks for getting to this! This definitely helps save time by reducing the need to rebase the PR just to sign off and rename them.

got have a few questions tho

Comment on lines +247 to +260
if (!hasValidPrefix) {
title = `build(dep): ${title}`;
console.log(`Updating PR title to: ${title}`);
const url = `https://api.github.com/repos/${owner}/${repo}/pulls/${pr.number}`;
await axios.patch(
url,
{ title },
{
headers: {
Authorization: `token ${githubToken}`,
},
},
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Ha this's a clever way to update the PRs.

I might be mistaken, but this isn’t quite what I initially envisioned. I thought Snyk is like a tool integrated into the repo, allowing us to reconfigure the Snyk PRs through their API rather than having to make patch requests to GitHub like this.

Have you confirmed that we can’t reconfigure Snyk through its API? Or maybe we should check with the CI team to ensure this is the only option?

Sorry, this approach feels a bit like a hack to me and I’d expect there to be a more streamlined way to configure the Snyk tool.

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 took a look initially but it seems very limited. It doesn't accept labels and we can pass only static strings with limited variables.

It can work for the title but we won't be able to detect if it's an Upgrade or a Security upgrade which Snyk currently does.
For the labels - we would still need to keep the hack or do them manually.

https://docs.snyk.io/scan-with-snyk/pull-requests/snyk-pull-or-merge-requests/customize-pr-templates/apply-a-custom-pr-template

https://docs.snyk.io/scan-with-snyk/pull-requests/snyk-pull-or-merge-requests/customize-pr-templates/variables-list-and-description

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see.

On a different note, how about we remove Snyk entirely and rely only on dependabot? I haven’t seen Snyk PRs in Mirror Node or Mirror Node Explorer anymore but only by dependa bots.

Would it be worth removing it altogether?
@beeradb @andrewb1269hg @rbarker-dev, what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal For changes that affect the project's internal workings but not its outward-facing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SMALL]Automatic bumping PRs are not properly structured
2 participants