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

Build and deploy with GitHub Actions #247

Merged
merged 4 commits into from
Apr 1, 2020
Merged

Conversation

beatngu13
Copy link
Contributor

To be discussed in tomorrow's innovations meeting.

run: |
mvn package -DskipTests
cd ./target/
echo "::set-output name=zip_name::$(echo recheck.cli-*-bin.zip)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use the same wildcard directly in L50?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I know … did some research and found this issue:

actions/upload-release-asset#4

Maintainer (@IAmHughes) suggests this approach.

@modulo11
Copy link
Contributor

I like the approach and only have some minor requests:

  • No need to use multiline when just one command is executed
  • What about matrix builds?
  • Test with and without caching the .m2 folder
  • Supply a way to externally change maven args e.g. via env variables like in the travis builds
  • Is there a neat way to upload to S3?

@beatngu13
Copy link
Contributor Author

beatngu13 commented Mar 31, 2020

I like the approach and only have some minor requests:

  • No need to use multiline when just one command is executed

This is from official examples like here:

https://github.com/actions/cache/blob/master/examples.md#java---maven

While multiline is indeed not needed, I think it is a bit more readable. But I don't really care as long as it is consistent.

  • What about matrix builds?

Supported, but we currently don't need them?

  • Test with and without caching the .m2 folder

Hmmm, why? It is already there and faster since we don't have to pull all dependencies all the time?

  • Supply a way to externally change maven args e.g. via env variables like in the travis builds

We can use secrets for this.

  • Is there a neat way to upload to S3?

There seem to be a bunch of actions, but for what do we need it here?

@enesoezel
Copy link
Contributor

I still have my github actions workflow from the presentation four months ago. I especially wanted to point out the asset upload, because it could be very useful for us in terms of downloading the golden master. Here starting at line 43.

@beatngu13
Copy link
Contributor Author

@oezelenes thx for pointing to actions/upload-artifact. I don't think it helps in the case of recheck.cli, but may be helpful when it comes to uploading test artifacts such as GMs or the like.

@beatngu13
Copy link
Contributor Author

@modulo11 without caching requires 30 – 40 s more:

https://github.com/retest/recheck.cli/actions/runs/67976320

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
@beatngu13 beatngu13 marked this pull request as ready for review April 1, 2020 10:11
@beatngu13 beatngu13 requested review from diba1013 and modulo11 April 1, 2020 10:11
Copy link

@diba1013 diba1013 left a comment

Choose a reason for hiding this comment

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

  1. What about matrix builds?

    Supported, but we currently don't need them?

    Since we do not ship a JDK currently, at least java 8 and 14? (e.g. my workflow)

  2. Supply a way to externally change maven args e.g. via env variables like in the travis builds

    We can use secrets for this.

    Do we still want to do this? If yes, do we need to distinguish build and deploy?

@beatngu13
Copy link
Contributor Author

Since we do not ship a JDK currently, at least java 8 and 14? (e.g. my workflow)

Java 14 and SonarCloud currently don't mix well (see e.g. here). We could run the analysis only if Java 8 is being used, however, we didn't do this with Travis either. The PR, as it is, is functionally equivalent with what we had before. Therefore, I would suggest to consider a build matrix in a separate step.

Do we still want to do this? If yes, do we need to distinguish build and deploy?

I would experiment with it over time and leave it as it is. It feels to me that using secrets for this purpose is not the right thing to do, but I can't come up with an alternative. Also, in Travis we were using environment variables as well, which are very similar (one could select visibility and they are not encrypted by default).

@beatngu13 beatngu13 requested a review from diba1013 April 1, 2020 13:15
@beatngu13 beatngu13 force-pushed the feature/github-actions branch from f4ee6e1 to 8d1810d Compare April 1, 2020 14:54
Copy link

@diba1013 diba1013 left a comment

Choose a reason for hiding this comment

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

Ok, let's try it out.

Please be sure to update the branch protection rules before merging this. Mergify potentially needs a refresh afterwards.

@beatngu13
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Apr 1, 2020

Command refresh: success

@mergify mergify bot merged commit d25c2e8 into master Apr 1, 2020
@mergify mergify bot deleted the feature/github-actions branch April 1, 2020 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants