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

Improve pipelines #5432

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

UlisesGascon
Copy link
Member

@UlisesGascon UlisesGascon commented Feb 2, 2024

Main Changes

This change include the pinning for the GItHub Actions dependencies (ce959ac) and the permissions definition for the pipeline (ea604fb)

Impact in the OSSF Scorecard

Screenshot 2024-02-02 at 16 46 53
Screenshot 2024-02-02 at 16 47 08

Context

Changes related

Team discussion related

Changelog

@UlisesGascon UlisesGascon marked this pull request as ready for review February 2, 2024 15:51
@UlisesGascon UlisesGascon mentioned this pull request Feb 2, 2024
@dougwilson
Copy link
Contributor

What is the npmCommand warnings referring to in that report output?

@UlisesGascon
Copy link
Member Author

What is the npmCommand warnings referring to in that report output?

I think the issue is with npm install --save-dev ${{ matrix.npm-i }}. I can ask the Scorecard Team for support on this, because I can see that the dependencies in the matrix.npm-i are pinned.

Take in account that until we don't merge #5431 we are depending on the OSSF Scorecard CRON job to update the results. So the last scanned commit was 0e3ab6e, here is the visual report

@wesleytodd
Copy link
Member

Do we know if the dependabot updates (assuming we decide to use that) for these versions will take into account semverness? I know it doesn't normally (which is soooooo annoying and broken) but I thought there was a setting to disallow majors, I am not sure that would work with commit hashes.

Not blocking at all, but the more we lean into these the more general maintenance we need to do. IMO a better way is to move this kind of stuff to a shared workflow in .github and keep all the packages in the orgs pointing to one individual source we need to update. Does that description make sense?

Copy link
Member

@inigomarquinez inigomarquinez left a comment

Choose a reason for hiding this comment

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

Added suggestions for latest versions of pinned GitHub actions.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@jonchurch
Copy link
Member

@wesleytodd

Do we know if the dependabot updates (assuming we decide to use that) for these versions will take into account semverness?

Dependabot should ignore deps pinned to commits. It just doesn't know what to do with them, so it will skip them.

@wesleytodd
Copy link
Member

So the problem then is we don't get any updates. Which is maybe fine, but still this is a bad DX imo.

@jonchurch
Copy link
Member

Pinning these deps is part of the scorecard score and guidance recommendations, hence why they have been proposed this way.

@wesleytodd
Copy link
Member

Yeah I get that, doesnt help make it any less of a UX problem though right? We still need to update them on a regular basis.

UlisesGascon and others added 3 commits April 1, 2024 13:15
Co-authored-by: Íñigo Marquínez Prado <25435858+inigomarquinez@users.noreply.github.com>
Co-authored-by: Íñigo Marquínez Prado <25435858+inigomarquinez@users.noreply.github.com>
Co-authored-by: Íñigo Marquínez Prado <25435858+inigomarquinez@users.noreply.github.com>
@UlisesGascon
Copy link
Member Author

@inigomarquinez I think that updating the dependencies might break the pipelines, I will have a look into it.

@UlisesGascon
Copy link
Member Author

Do we know if the dependabot updates (assuming we decide to use that) for these versions will take into account semverness?

Yes, it should support this approach. The comment is just to make it more human readable. The reason why we need to use commit hashes and not the tag reference directly is because the tags are not immutable so we can end up running something different that what we expect.

More context about the attack vector

@inigomarquinez
Copy link
Member

dependabot does update pinned dependencies. At least in all the projects where I use pinned dependencies in GitHub actions along with dependabot, it opens a new pull request to update them to the latest pinned dependency. It even updates the comment saying to which version it belongs to.

@wesleytodd

Do we know if the dependabot updates (assuming we decide to use that) for these versions will take into account semverness?

Dependabot should ignore deps pinned to commits. It just doesn't know what to do with them, so it will skip them.

@wesleytodd
Copy link
Member

Frankly seeing this same change in many repos is going to not be something I am interested in dealing with. I am border line on this, but I think I am leaning toward: This shouldn't land until there is a way to centrally manage this across all the repos with a single update.

Which I think means that we should refactor many of the CICD setups to use shared worfklows from the .github repo.

@UlisesGascon UlisesGascon added the semver-ignore This change does not have any impact in semver (docs, tooling, etc..) label Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-ignore This change does not have any impact in semver (docs, tooling, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants