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

Update sonatype permissions #55

Merged
merged 2 commits into from
Nov 6, 2018

Conversation

whikloj
Copy link
Member

@whikloj whikloj commented Oct 29, 2018

GitHub Issue: N/A

What does this Pull Request do?

Updates the sonatype environment variables and (hopefully) allows us to release the snapshot builds.

What's new?

  • Does this change require documentation to be updated? no
  • Does this change add any new dependencies? no
  • Does this change require any other modifications to be made to the repository (ie. Regeneration activity, etc.)? no
  • Could this change impact execution of existing code? no

How should this be tested?

Travis is Green? Supergreen.

Interested parties

@Islandora-CLAW/committers

@codecov
Copy link

codecov bot commented Oct 29, 2018

Codecov Report

Merging #55 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #55   +/-   ##
=========================================
  Coverage     80.36%   80.36%           
  Complexity       93       93           
=========================================
  Files            17       17           
  Lines           331      331           
  Branches          1        1           
=========================================
  Hits            266      266           
  Misses           64       64           
  Partials          1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b5af5b...1ce5ba6. Read the comment docs.

@acoburn
Copy link
Contributor

acoburn commented Oct 29, 2018

I just did a similar thing on the Trellis repos just last week. It looks like this configuration will work fine, but there are two considerations for things to add.

One is that, when it comes time to release the artifacts, you might not want the automated build of the release commit to interfere with the locally-initiated ./gradlew release process (one set of artifacts will be signed, and therefore valid for Maven central, while the other set won't be signed). I found it easiest to add a getVersion task in the build.gradle file and add a filter to the script stage.

The other consideration is that, periodically, you may find that pushing the build to Sonatype will fail -- there may be weird, transient network issues. Given that, the entire build will fail with this configuration. If that's what you want, then leave this as it is. My perspective is that the tests need to pass, but pushing to Sonatype (and Docker) is just a convenience, so I allow those to fail (even though they succeed 95% of the time).

@acoburn
Copy link
Contributor

acoburn commented Oct 29, 2018

BTW, I trust that the encrypted username/password here are revokable. That is, Sonatype will provide a revokable username/password token for a given user so that you don't need to provide Travis-CI with a real username/password combination. I have also found that putting these values in environmental variables might be somewhat easier -- if you need or want to rotate your keys every N months, you won't need to send a new commit just for that purpose: you can just change the settings on the Travis build directly.

@whikloj
Copy link
Member Author

whikloj commented Oct 29, 2018

I'm not sure about the release versus snapshot release issue, I'll defer to @jonathangreen and @dannylamb on that one.

I did use my account information, so thank you for that. I'll swap it out but also see about adding it to the Travis setup instead. 👍

@whikloj
Copy link
Member Author

whikloj commented Oct 29, 2018

Hmm to use encrypted environment variables, the PRs have to come from the same repo. So this will not deploy anything when the PR is from a forked repository. https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions

Not that I consider this a problem, just something to be aware of.

@acoburn
Copy link
Contributor

acoburn commented Oct 29, 2018

Though you wouldn't want the pull-request to trigger a deployment, correct? Only after the PR is merged should the deploy stage factor in at all.

@whikloj
Copy link
Member Author

whikloj commented Oct 29, 2018

@acoburn I'm not really clear how we were using these snapshot deployments in the first place, so it is possible this is how it was working before. In which case 👏 we're good 😄

@acoburn
Copy link
Contributor

acoburn commented Oct 29, 2018

@whikloj I agree. I think this looks good.

@dannylamb
Copy link
Contributor

My understanding was the deploy stage isn't triggered on pull request builds, only after they are merged. That's a Travis thing I should probably confirm.

PR is good and I'm pulling it in. Thx @whikloj and @acoburn.

Copy link
Contributor

@dannylamb dannylamb left a comment

Choose a reason for hiding this comment

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

:shipit:

@dannylamb dannylamb merged commit 3a24020 into Islandora:master Nov 6, 2018
@whikloj whikloj deleted the update-sonatype-permissions branch November 11, 2018 20:24
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.

3 participants