Skip to content

feat: Publish maven packages without use of Gradle - #351

Merged
kamilogorek merged 4 commits into
masterfrom
maven-central-release
Feb 14, 2022
Merged

kamilogorek merged 4 commits into
masterfrom
maven-central-release

Conversation

@kamilogorek

Copy link
Copy Markdown
Contributor

This PR utilizes direct API calls to close and publish packages to Nexus, instead of going through a 3rd party Gradle plugin.

Tests and verification are still TBD, but it's already EOD, and wanted to get this PR out for an initial review.

This change will also make #278 obsolete.

Comment thread src/targets/maven.ts Outdated
@bruno-garcia
bruno-garcia self-requested a review February 11, 2022 18:49
Comment thread src/targets/maven.ts
Comment thread src/targets/maven.ts Outdated
Comment thread src/targets/maven.ts Outdated
Comment thread src/targets/maven.ts Outdated
Comment thread src/targets/maven.ts
Comment thread src/targets/maven.ts
Comment thread src/targets/maven.ts Outdated
Comment thread src/targets/maven.ts Outdated
@kamilogorek
kamilogorek enabled auto-merge (squash) February 14, 2022 12:07
@kamilogorek
kamilogorek merged commit ada2954 into master Feb 14, 2022
@kamilogorek
kamilogorek deleted the maven-central-release branch February 14, 2022 12:07
Comment thread src/targets/__tests__/maven.test.ts
Comment thread src/targets/__tests__/maven.test.ts
Comment on lines +368 to +372
try {
await mvnTarget.closeAndReleaseRepository();
} catch (e) {
// no-empty
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not something like expect(...).not.toThrow()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because we actually want to throw here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then shouldn't it be more like:

expect(mvnTarget.closeAndReleaseRepository).rejects.toThrowErrorMatchingInlineSnapshot()

?

Right now there's nothing asserting that we have thrown anything.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because imo assertions should match the name of the test.

should not release repostiory if it was not closed properly

And assertion is that releaseRepository was never called. closeAndReleaseRepository is only a setup trigger.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Because imo assertions should match the name of the test

Yeah, that makes sense.

And assertion is that releaseRepository was never called. closeAndReleaseRepository is only a setup trigger.

I think a comment here instead of // on-empty would go a long way to communicate this to other developers such as myself and @iker-barriocanal 馃檪

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair point, will keep that in mind :)

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.

4 participants