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

Normalize Swift package names #7648

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

deivid-rodriguez
Copy link
Contributor

In other places in Github, like in Security Advisories, or the Dependency Graph page, a different naming schema is used.

This means that when we try to find security updates to resolve a certain alert for a dependency, we won't be able to match the dependency with the advisory in the alert, and Dependabot will see it as non vulnerable.

So, normalize the schema to what the rest of GitHub uses.

@github-actions github-actions bot added the L: swift Swift packages label Jul 27, 2023
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/swift-name-normalization branch 2 times, most recently from 0aaadcf to 9988b15 Compare July 27, 2023 12:33
@deivid-rodriguez
Copy link
Contributor Author

This will require an update to the smoke tests but other than that I think it's ready.

Open questions:

  • In the second commit I made an extra change to keep current PR titles using the short name, but we may want to keep the one starting with github.com/.
  • I used metadata to pass the "identifier" around, since we need to pass that to the swift package update command to update specific packages, and swift does not know about our naming scheme. An alternative to using metadata would be to use a display_name builder like the one for Gradle for example, and deduce it from the long name.

Thoughts?

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/swift-name-normalization branch from 9988b15 to 53694e4 Compare July 27, 2023 17:02
@deivid-rodriguez
Copy link
Contributor Author

I believe changing the naming schema here is also going to force users to use the github.com/reactivecocoa/reactiveswift naming schema in their ignore conditions?

I'm not sure that's ideal, but if we go that route, we should also use github.com/reactivecocoa/reactiveswift in PR titles and bodies to not make things more confusing.

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Jul 31, 2023

We believe the best path forward is to stick to full names, at least for now.

Last open question is regarding the implementation:

I used metadata to pass around the "identifier" dumped by swift package show-dependencies --format json around, since we need to pass identifier to the swift package update and swift package resolve commands to update specific packages, and swift does not know about our naming scheme.

An alternative to using metadata would be to use add some custom logic right before passing it to the package manager (like name.split("/").last).

Letting the package manager "decide" the logic to build the "identifier" for a dependency feels better, but it does require more work to allow passing that metadata around everywhere.

In other places in Github, like in Security Advisories, or the
Dependency Graph page, a different naming schema is used.

This means that when we try to find security updates to resolve a
certain alert for a dependency, we won't be able to match the dependency
with the advisory in the alert, and Dependabot will see it as non
vulnerable.

So, normalize the schema to what the rest of GitHub uses.
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/swift-name-normalization branch from 24fe813 to f1cb29c Compare July 31, 2023 14:37
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review July 31, 2023 15:02
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner July 31, 2023 15:02
@deivid-rodriguez
Copy link
Contributor Author

This should go green once dependabot/smoke-tests#112 is merged.

Copy link
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

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

Makes sense.

I suppose we could do dependency.name.split('/').last but I like this better in case identity changes in a future version of Swift.

@jakecoffman
Copy link
Member

Oh your last comment says exactly the same, so yes we agree!

@deivid-rodriguez deivid-rodriguez merged commit 8c107a5 into main Jul 31, 2023
107 checks passed
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/swift-name-normalization branch July 31, 2023 17:34
deivid-rodriguez added a commit that referenced this pull request Aug 3, 2023
brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
In other places in Github, like in Security Advisories, or the
Dependency Graph page, a different naming schema is used.

This means that when we try to find security updates to resolve a
certain alert for a dependency, we won't be able to match the dependency
with the advisory in the alert, and Dependabot will see it as non
vulnerable.

So, normalize the schema to what the rest of GitHub uses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: swift Swift packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants