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

Skip SDK upgrades that are pinned to a specific commit #6238

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

Conversation

Victorsesan
Copy link

No description provided.

…sable updates when the dependency is pinned to a specific commit or digest. The final rule ensures no updates are applied if the dependency's version source URL is a commit SHA.

Related to open-telemetry#6192
@Victorsesan Victorsesan requested a review from a team as a code owner October 13, 2024 01:29
@dmathieu
Copy link
Member

This also changes some unrelated formatting of the file, making the diff harder to read. Could you remove those unrelated formats ?

@Victorsesan
Copy link
Author

@dmathieu I've maintained the fille structure and ensured formatting is consistent and easy to read I'm not sure if that fixed it. Also the renovate bot link; renovate-schema.js file is unable to load from my end not sure why but maybe it could be the problem. Please let me know if that changed anything, thanks

renovate.json Outdated Show resolved Hide resolved
renovate.json Show resolved Hide resolved
renovate.json Outdated Show resolved Hide resolved
renovate.json Outdated Show resolved Hide resolved
renovate.json Outdated Show resolved Hide resolved
renovate.json Show resolved Hide resolved
renovate.json Show resolved Hide resolved
@dmathieu dmathieu changed the title Aded an ignore property under the matchPackageNames rule to disable updates when the dependency is pinned to a specific commit or digest Skip SDK upgrades that are pinned to a specific commit Oct 16, 2024
Victorsesan and others added 3 commits October 17, 2024 01:55
Co-authored-by: Damien Mathieu <[email protected]>
Co-authored-by: Damien Mathieu <[email protected]>
Co-authored-by: Damien Mathieu <[email protected]>
Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

Left two nits.

renovate.json Outdated
}
},
{
"_description": "Any SDK upgrade that made it through here is a pin/digest. DIsable it",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"_description": "Any SDK upgrade that made it through here is a pin/digest. DIsable it",
"_description": "Any SDK upgrade that made it through here is a pin/digest. Disable it",

renovate.json Outdated
@@ -31,8 +27,18 @@
"groupName": "golang.org/x"
},
{
"_description": "Ignore pin and digest upgrades. We only want published releases of the SDK to trigger upgrades",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"_description": "Ignore pin and digest upgrades. We only want published releases of the SDK to trigger upgrades",
"_description": "Ignore pin and digest upgrades. We only want published releases of the SDK to trigger upgrades",

@dmathieu dmathieu added the Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG label Oct 17, 2024
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.2%. Comparing base (856523b) to head (890e925).
Report is 7 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6238     +/-   ##
=======================================
+ Coverage   66.9%   67.2%   +0.2%     
=======================================
  Files        190     191      +1     
  Lines      12540   12637     +97     
=======================================
+ Hits        8395    8494     +99     
+ Misses      3855    3854      -1     
+ Partials     290     289      -1     

see 6 files with indirect coverage changes

renovate.json Outdated
"allowedVersions": "/^v[0-9]+\\.[0-9]+\\.[0-9]+/"
"enabled": true,
"ignore": {
"matchUpdateTypes": ["pin", "digest"]
Copy link
Member

Choose a reason for hiding this comment

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

What is a pin?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like it's a dependency set at an explicit version.
https://docs.renovatebot.com/dependency-pinning/

Pinning isn't something we can really do with Go. But just to be same, maybe we should actually remove that match.

Copy link
Member

@pellared pellared Oct 18, 2024

Choose a reason for hiding this comment

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

I would prefer this PR to be tested on a fork.
@Victorsesan, can you test it on https://github.com/Victorsesan/opentelemetry-go-contrib?

Copy link
Author

Choose a reason for hiding this comment

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

@pellared I would be happy to! sorry for messing it up , i'm trying but my PR keeps getting tested here instead can you please give me some pointers on how to test directly on my fork repo . thanks

Copy link
Author

Choose a reason for hiding this comment

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

@dmathieu Did we succeeded in fixing this issue i would love a feedback if there is anything else i need to do i will be happy to! Thanks

Copy link
Author

Choose a reason for hiding this comment

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

@pellared @dmathieu Are these the expected test we need to see? Thanks for the time
Screenshot 2024-10-24 004430
Screenshot 2024-10-24 004348
Screenshot 2024-10-24 004059

Copy link
Member

Choose a reason for hiding this comment

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

They would be, yes. But in your screenshot, they appear to say the configuration file is invalid.

Copy link
Author

Choose a reason for hiding this comment

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

@dmathieu Finally succeeded in making renovate work as expected after a long try and errors, i had to make some minor changes for it to pass the test which I'm not sure if its good or bad but i mentioned all the errors i faced with the solution and modifications i did in my recent commit which i will be happy if you can check it out and let me know if we are in the right track. I would love to succeed in working on this . Thank you all for your time.
Screenshot 2024-10-25 010858
Screenshot 2024-10-25 010932

Copy link
Member

Choose a reason for hiding this comment

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

This shows the config is valid. But it doesn't seem to validate that commit upgrades would be skipped, while releases would be bumped?

Copy link
Author

Choose a reason for hiding this comment

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

hows the config is valid. But it doesn't seem to validate that commit upgrades would be skipped, while releases would be bumped

@dmathieu Sorry i missed that, is this last commit good i have added something to fix it. I was just worried with mend not accepting the package rules even when they are good sometimes which made me lost track validating the skip rule and rather focus on having renovate pass the test, but let me know if the few changes i made does it and it did also pass the test. thanks

Victorsesan and others added 9 commits October 19, 2024 01:40
Problems i found with mend.io
-After runnig the first test, the errors indicated that certain options are invalid, specifically the _description and ignore fields in my packageRules which i had to change and remove.
*Solution: I removed _description Fields as they were classified as not a valid options in the Renovate configuration.
*Kept the ignore Field which was valid, but ensured that it is used correctly. In this case, it remains in the rule for go.opentelemetry.io/otel/**

-I ran a second test which had another error. The ignore field in packageRules[4] was still causing issues. The ignore option was not valid in the context of packageRules.
*Solution: I used instead an enabled field to control whether updates are applied.
…mit upgrades. This will ensure that any dependency that is not explicitly allowed will be skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants