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

[CA-1327] Github Actions #521

Merged
merged 33 commits into from
May 7, 2021
Merged

[CA-1327] Github Actions #521

merged 33 commits into from
May 7, 2021

Conversation

s-rubenstein
Copy link
Contributor

Ticket:


PR checklist

@coveralls
Copy link

coveralls commented Apr 21, 2021

Coverage Status

Coverage remained the same at 78.54% when pulling 62f9e72 on sr-github-actions into c34ff18 on develop.

@s-rubenstein
Copy link
Contributor Author

I will make the Travis CI piece not required, and make the GHA required before merging, just that is a repo-wide setting so wanted to wait for approvals before futzing with it.

@andy7i
Copy link
Contributor

andy7i commented Apr 28, 2021

could you add a bit more details about the PR: any differences between travis vs GHA that we should know about? (also, I think we're required to have a ticket # for each PR now)

@s-rubenstein s-rubenstein changed the title Test github actions [no review] [CA-1327] Github Actions Apr 29, 2021
@s-rubenstein
Copy link
Contributor Author

Yes, thank you. Forgot to update the title after requesting review.

The big differences between GHA and Travis are:

  1. There's no separate script for publishing the client library, instead we have some branching logic in the actions file.
  2. Test logic and client logic have been separated into two jobs for clarity
  3. Artifactory login information is stored in GitHub secrets rather than in Travis env variables.

.github/workflows/build.yml Show resolved Hide resolved
id: tests
run: sbt clean "testOnly -- -n org.broadinstitute.tags.SchemaInit"

- name: Generate coverage report
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this runs the unit tests and generates the coverage report

./minnie-kenny.sh --force
git secrets --scan-history

- name: Integration tests
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: these aren't integration tests, this command just runs the tests tagged with SchemaInit. Maybe rename to 'Run JndiSchemaInit tests' or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, can rename.

.github/workflows/build.yml Show resolved Hide resolved

- name: Publish java client for merge
working-directory: codegen_java
id: publishJavaClientSnapshot
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the ids are swapped, this one isn't a snapshot

.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
- bash scripts/gen_java_client.sh
after_success:
- export SBT_OPTS="-J-Xmx3g"
- sbt coveralls
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we can remove this dependency

addSbtPlugin("org.scoverage" % "sbt-coveralls" % "1.2.5")

.github/workflows/build.yml Outdated Show resolved Hide resolved
s-rubenstein and others added 2 commits April 30, 2021 13:54
Co-authored-by: Andrew <andy7i@users.noreply.github.com>
Copy link
Contributor

@andy7i andy7i left a comment

Choose a reason for hiding this comment

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

👍 thanks for making these changes / answering questions!

@s-rubenstein s-rubenstein merged commit c93b987 into develop May 7, 2021
@s-rubenstein s-rubenstein deleted the sr-github-actions branch May 7, 2021 20:23
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.

None yet

4 participants