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

Provide an option to by pass generating a settings.xml file #79

Closed
jvanzyl opened this issue Jun 29, 2020 · 9 comments
Closed

Provide an option to by pass generating a settings.xml file #79

jvanzyl opened this issue Jun 29, 2020 · 9 comments
Labels
feature request New feature or request to improve the current logic

Comments

@jvanzyl
Copy link

jvanzyl commented Jun 29, 2020

When using self-hosted runners we provision compute with a Maven settings.xml and currently this action will overwrite our correct settings.xml with one that's inoperable. It would be nice to signal to the action to not generate a settings.xml file.

Another option might be to allow a template .github/workflow/maven/settings.xml that can be provisioned on the runner. In the case of running with GitHub actions proper the template can use ${{ secrets.references }} and in self-host just normal envars which Maven will interpolate into the settings file. Either way it's secure.

@brunoborges
Copy link

I'd say that ideally, the default should be not to generate settings.xml.

@jvanzyl
Copy link
Author

jvanzyl commented Jul 15, 2020

If that is possible, sure. I assume there is an existing use case internally that conflicts with this.

I think the cases where it can safely generate a setting.xml are:

  1. if there is a settings template in a known location, it can be rendered on to GHA node

  2. if there are secrets with standard names to be used in a generated settings.xml, render a settings.xml to the GHA node

@maxim-lobanov
Copy link
Contributor

Hello everyone!
This request was implemented in v2-preview release via #136
You can test it right now:

- name: Setup Java
  uses: actions/setup-java@v2-preview
  with:
    distribution: 'adopt'
    java-version: '11'
    server-id: maven
    server-username: MAVEN_USERNAME
    server-password: MAVEN_CENTRAL_TOKEN
    gpg-private-key: ${{ secrets.MAVEN_GPG_PRIVATE_KEY }}
    gpg-passphrase: MAVEN_GPG_PASSPHRASE
    overwrite-settings: false

overwrite-settings: false does the magic.
We are going to release v2 stable by the end of next week but we would be very appreciate for any testing of v2-preview and confirmations that it works as expected.

@anderius
Copy link

anderius commented Mar 24, 2021

I am still at loss as to why the action deals with Maven at all. Why should installing java involve anything with Maven?

@brunoborges
Copy link

@maxim-lobanov could you please share again the reasoning behind the Maven specific functionality?

@maxim-lobanov
Copy link
Contributor

I don't have much historical context but @konradpabjan has shared some details in #132 (comment)

The main reason why setup-java has Maven functionality in it is so that it's easier to publish to GitHub package registry. When the initial maven publishing feature was added there was a pretty lengthy debate (and I was very much against adding the functionality to setup-java) but we ultimately decided to do it so that publishing would "just work" without any extra configurations or any other actions. I agree that it somewhat distorts the single purpose intent that setup-java initially was meant to provide (and that most users think it provides) but we ultimately decided that the pros outweight the cons.
One solution that was floating around was to create a separate setup-maven or setup-gradle package but we don't have the bandwidth to support yet another action and the maintenance burden would also be very high so we just extended setup-java
Before we started work on the v2-preview, we had some discussions regarding what to do with the existing Maven publishing functionality and the general consensus was to just leave it as is. Don't prioritize any new features for it, don't break what we currently have, but keep the current functionality as is because we still want publishing to GitHub packages to "just work".

@konradpabjan
Copy link
Collaborator

I will also add that support was expanded not just for Github package registry (you can publish to any Maven Central Repository). See this guide: https://docs.github.com/en/actions/guides/publishing-java-packages-with-gradle

setup-java does all the magic and the only thing the user has to do is gradle publish

      - name: Publish package
        run: gradle publish
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

Without the Maven logic in setup-java there would be a lot more configuration needed for users. Easier publishing at the cost of Maven specific logic being added to setup-java was a tradeoff that we just decided to make.

@brunoborges
Copy link

Without the Maven logic in setup-java there would be a lot more configuration needed for users. Easier publishing at the cost of Maven specific logic being added to setup-java was a tradeoff that we just decided to make.

My concern with this thinking is that it opens the door to add a lot of other features.

More importantly, how is this impacted by a build that uses a matrix of multiple Java versions? Will settings.xml be updated every time for each version? What if I want to publish 2+ times, to different locations (e.g. private repo, public repo, staging repo)?

@maxim-lobanov
Copy link
Contributor

This concern is fair enough and we definitely need to be very selective for new features in future.
Unfortunately, we can't deprecate this functionality or switch it to be disabled by default without significant impact for customers. So we have added overwrite-settings: false that provides way to disable this functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants