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

Closes #55 #57

Merged
merged 1 commit into from
May 13, 2021
Merged

Closes #55 #57

merged 1 commit into from
May 13, 2021

Conversation

Smasherr
Copy link
Contributor

@Smasherr Smasherr commented May 6, 2021

Summary

Add support for encrypted maven credentials.

Use Case

Usage of encrypted maven credentials (settings.xml plus settings-security.xml).

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@Smasherr Smasherr requested a review from a team May 6, 2021 21:55
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 6, 2021

CLA Signed

The committers are authorized under a signed CLA.

@Smasherr Smasherr force-pushed the main branch 4 times, most recently from 8c26922 to dfc0495 Compare May 10, 2021 11:15
Copy link
Contributor

@dmikusa dmikusa left a comment

Choose a reason for hiding this comment

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

I didn't try running yet, but one request based on a quick read-through of the changes. It looks like you modified the existing test case to check for both files. Would you be able to make two separate test cases?

I'd prefer if the existing test case remains the same & an additional test case is added validating the new scenario. That way we're validating both cases, i.e. where a.) just a settings.xml is provided and b.) both settings.xml and settings-security.xml are provided.

@dmikusa dmikusa added semver:minor A change requiring a minor version bump type:enhancement A general enhancement labels May 10, 2021
@Smasherr
Copy link
Contributor Author

@dmikusa-pivotal thanks for your suggestion. I've reverted now the original test case and added a second one as you proposed.

@Smasherr Smasherr requested a review from dmikusa May 11, 2021 16:57
Copy link
Contributor

@dmikusa dmikusa 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. Unit tests pass for me. Changed a few things and they broke as expected. Tested some builds with and without a security file. All worked as expected. Thanks for the PR!

@dmikusa
Copy link
Contributor

dmikusa commented May 13, 2021

Github is giving me issues merging. As soon as these clear up, I will merge this. Sorry for the delay.

@Smasherr
Copy link
Contributor Author

Github is giving me issues merging. As soon as these clear up, I will merge this. Sorry for the delay.

No worries, thank you for your quick operation!

@dmikusa dmikusa merged commit cad0674 into paketo-buildpacks:main May 13, 2021
@Smasherr
Copy link
Contributor Author

Smasherr commented May 14, 2021

Just one more question: what's the release cycle of this buildpack?

@dmikusa
Copy link
Contributor

dmikusa commented May 14, 2021

We don't have a time-based release cycle, it's more or less when there's something to release, we'll cut a new release. We are trying to iterate fast and get new features out quickly.

I'll release this one today. It'll take a little time to get picked up by the paketo-buildpacks/java buildpack & the builder (maybe a day or two). You can use it directly in the meantime with pack build -b.

Hope that helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants