Marvelution / mpac-version-publisher

Apache License 2.0
1 stars 0 forks source link

Split into library - fixes #2 #5

Closed EugenMayer closed 1 month ago

EugenMayer commented 1 month ago

Split project into 'core' and 'cli' modules - fixed #2

markrekveld commented 1 month ago

I pushed a tab character fix to main, a simple rebase should get this branch back in order

markrekveld commented 1 month ago

@EugenMayer the main test classes BaseTestPublishToMarketplace.java , CommandLinePublishToMarketplaceTest, EnvironmentVariablesPublishToMarketplaceTest are not in a java source directory so are not executed. Once they are in a test source directory, they will also need to have classpath access the fake-jira and fake-marketplace test resources for the core package. This can be done by adding the core tests artifact as test dependency on the cli module.

EugenMayer commented 1 month ago

@markrekveld not sure why you pushed a code-formatting commit on main before merging all branches, but the above merge conflict is yours to take please :)

markrekveld commented 1 month ago

not sure why you pushed a code-formatting commit on main before merging all branches

So that your P doesn't include reformatting, making it easier to review. I pushed a commit with the rebase

EugenMayer commented 1 month ago

we will need to rebase the other PR too, i can try to do that. Beside that, i was not going to change code here besides imports if any possible, so no formatting. Is there any way to share your codestyles so we can be sure we work with the same?

markrekveld commented 1 month ago

@EugenMayer I have this repo where I store my code style profiles. All are exported from my IntelliJ IDEA

markrekveld commented 1 month ago

we will need to rebase the other PR too, i can try to do that.

True, you can do it if you like, or leave it to me since I'll be updating the PR anyway to support my maven repositories

EugenMayer commented 1 month ago

@markrekveld i would love to finish this one up - are any things open for you or could we potentially plan a merge?

markrekveld commented 1 month ago

I updated the GH action to runon worklfow_dispatch instead of push since I have not yet had the chance to update the CI/CD infra to support GH

EugenMayer commented 1 month ago

do you mean your nexus by that? if yes, i would rather stop deploying snapshots but let the gh action run, so we have tests and builds. What do you think?

markrekveld commented 1 month ago

do you mean your nexus by that? if yes, i would rather stop deploying snapshots but let the gh action run, so we have tests and builds. What do you think?

Yeah, I need to provision temporal permissions for GH and don't have the time right now, its a bit more then just enabling. If the build can run without issue, then sure we can enable it for push triggers.

EugenMayer commented 1 month ago

This PR just builds, it does not deploy - so for this PR you do not need to disable it at all. Could you just enable it again and we might just wait mergin the other pr (release/publihs) pr until you find time to provision the secerts

markrekveld commented 1 month ago

This PR just builds, it does not deploy - so for this PR you do not need to disable it at all. Could you just enable it again and we might just wait mergin the other pr (release/publihs) pr until you find time to provision the secerts

I just enabled push builds again.

EugenMayer commented 1 month ago

Thanks. So are we good top go then? (just give your blessing via a review as soon as you have time and i merge it)

markrekveld commented 1 month ago

I needed to update the review ruleset and strait afterwards merged it, sorry I read over the 'i can merge' part of your last comment.

EugenMayer commented 1 month ago

no worries, i do not mind at all. Work is done - me happy!