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

make gradle display_name property use full dependency name #6526

Merged
merged 2 commits into from
Jan 29, 2023
Merged

make gradle display_name property use full dependency name #6526

merged 2 commits into from
Jan 29, 2023

Conversation

mrwhoknows55
Copy link
Contributor

closes #3353

@mrwhoknows55 mrwhoknows55 requested a review from a team as a code owner January 26, 2023 17:04
Copy link
Member

@jeffwidman jeffwidman 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 tackling this!

What you've got looks good, but I realized while reviewing that these can be quite verbose, so we'll probably need to cap a max length... should be trivial to implement, but gimme a day or two to collect more info for #3353 (comment).

@jeffwidman
Copy link
Member

jeffwidman commented Jan 26, 2023

@mrwhoknows55 want to pick one of these solutions and run with it?:

So there's a lot of variables there, but if we just cap max length of the full dependency name at 100 chars that's probably good enough.

For when it's over 100 chars, then if we want to get fancy, truncate the path, ie full-pa../package-name. But given that will be rare, the simpler option is to punt and if longer then 100 chars revert back to current behavior of only displaying package name. I'm personally fine either way, we can always further improve it later if needed.

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@jeffwidman
Copy link
Member

Can you fix the lint failures? https://github.com/dependabot/dependabot-core/actions/runs/4025933944/jobs/6925118702

For the line too long, I'm fine if you disable that check for that line, IMO the point is to have a long package name...

method(argument) # rubocop:disable Layout/LineLength

I suspect the smoke test failure is because the smoke test needs updating to match the new expected behavior--I can handle that post merge.

@jeffwidman jeffwidman merged commit 5eff308 into dependabot:main Jan 29, 2023
@jeffwidman
Copy link
Member

I went ahead and fixed the lints so we could land this.

jeffwidman added a commit to dependabot/smoke-tests that referenced this pull request Jan 29, 2023
In dependabot/dependabot-core#6526, the display
name for Gradle was updated to include the full name. See that PR for
rationale.

Updated the smoke test by finding the image name from:
https://github.com/dependabot/dependabot-core/actions/runs/4034930902/attempts/1#summary-10953476551

And then running:
```shell
dependabot --updater-image ghcr.io/dependabot/dependabot-updater-gradle:5eff30823a1eb0aba02f2267d14427a8bb547414 test -f tests/smoke-gradle.yaml -o tests/smoke-gradle.yaml
```

And committing the diff.
@jeffwidman
Copy link
Member

I fixed the failing smoke test in dependabot/smoke-tests#27.

jeffwidman added a commit to dependabot/smoke-tests that referenced this pull request Jan 29, 2023
In dependabot/dependabot-core#6526, the display
name for Gradle was updated to include the full name. See that PR for
rationale.

Updated the smoke test by finding the image name from:
https://github.com/dependabot/dependabot-core/actions/runs/4034930902/attempts/1#summary-10953476551

And then running:
```shell
dependabot --updater-image ghcr.io/dependabot/dependabot-updater-gradle:5eff30823a1eb0aba02f2267d14427a8bb547414 test -f tests/smoke-gradle.yaml -o tests/smoke-gradle.yaml
```

And committing the diff.
alcere pushed a commit that referenced this pull request Feb 20, 2023
Make Gradle `display_name` property use full dependency name if less than 100 chars long.

---------

Co-authored-by: Jeff Widman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow option to use full dependency name for gradle display_name property
2 participants