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

Fixed merge-publish and disabled circleci publish workflow #45

Merged
merged 10 commits into from
Dec 22, 2020

Conversation

JBAhire
Copy link
Member

@JBAhire JBAhire commented Dec 18, 2020

Description

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Checklist:

  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Documentation

@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #45 (603275c) into main (fc6f400) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main      #45   +/-   ##
=========================================
  Coverage     80.71%   80.71%           
- Complexity      222      245   +23     
=========================================
  Files            27       27           
  Lines           752      752           
  Branches         56       56           
=========================================
  Hits            607      607           
  Misses           98       98           
  Partials         47       47           
Flag Coverage Δ Complexity Δ
integration 69.51% <ø> (-11.21%) 0.00 <ø> (ø)
unit 67.77% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 fc6f400...603275c. Read the comment docs.

.github/workflows/pr-build.yml Show resolved Hide resolved
.github/workflows/pr-build.yml Outdated Show resolved Hide resolved
steps:
- name: Check out code
uses: actions/checkout@v2.3.4
with:
Copy link
Contributor

@kotharironak kotharironak Dec 18, 2020

Choose a reason for hiding this comment

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

We will need the below condition to take changes from the forked pull request otherwise it will checkout only the main branch code

with:
        ref: ${{github.event.pull_request.head.ref}}
        repository: ${{github.event.pull_request.head.repo.full_name}}

Copy link
Member Author

Choose a reason for hiding this comment

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

yup. forgot about this earlier

@aaron-steinfeld
Copy link
Contributor

Another issue I forgot til now - the tag generation for docker non-release images is based off the circle ci branch - we should update that:
Ref: https://github.com/hypertrace/hypertrace-gradle-docker-plugins/blob/c4cbaa615a03724c17c9eed0decf7311ac8128dc/hypertrace-gradle-docker-plugin/src/main/java/org/hypertrace/gradle/docker/DockerPlugin.java#L111-L119

@kotharironak
Copy link
Contributor

https://github.com/hypertrace/hypertrace-gradle-docker-plugins/blob/c4cbaa615a03724c17c9eed0decf7311ac8128dc/hypertrace-gradle-docker-plugin/src/main/java/org/hypertrace/gradle/docker/DockerPlugin.java#L111-L119

As we will have to publish docker artifacts on main, we can do that for the on push event trigger of the main branch. Based on this doc -
https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions#github-context, we can derive it from GITHUB_REF for the above on push event.

we will have to expose a branch GITHUB_BRANCH to the merge-publish step, so it can be read in the gradle plugin. And we can do this by adding one more step like below before merge-publish

if: github.event_name == 'push' 
	 run: | 
	 GHA_BRANCH="${GITHUB_REF#refs/heads/}"
	 echo "GHA_BRANCH=${BRANCH}" >> $GITHUB_ENV

.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/pr-build.yml Show resolved Hide resolved
.github/workflows/pr-test.yml Outdated Show resolved Hide resolved
@JBAhire
Copy link
Member Author

JBAhire commented Dec 21, 2020

https://github.com/hypertrace/hypertrace-gradle-docker-plugins/blob/c4cbaa615a03724c17c9eed0decf7311ac8128dc/hypertrace-gradle-docker-plugin/src/main/java/org/hypertrace/gradle/docker/DockerPlugin.java#L111-L119

As we will have to publish docker artifacts on main, we can do that for the on push event trigger of the main branch. Based on this doc -
https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions#github-context, we can derive it from GITHUB_REF for the above on push event.

we will have to expose a branch GITHUB_BRANCH to the merge-publish step, so it can be read in the gradle plugin. And we can do this by adding one more step like below before merge-publish

if: github.event_name == 'push' 
	 run: | 
	 GHA_BRANCH="${GITHUB_REF#refs/heads/}"
	 echo "GHA_BRANCH=${BRANCH}" >> $GITHUB_ENV

addressed by hypertrace/hypertrace-gradle-docker-plugins#21

.github/workflows/pr-test.yml Outdated Show resolved Hide resolved
.github/workflows/pr-test.yml Show resolved Hide resolved
run: ./gradlew jacocoIntegrationTestReport

- name: Copy integration test reports
run: ./gradlew copyAllReports --output-dir=/tmp/integration-test-reports
Copy link
Contributor

Choose a reason for hiding this comment

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

The artifacts produced by this are the same as the artifacts reported by the previous step I believe since we're not cleaning out. We can either clean between the two test phases, or a single upload for both runs is probably fine too.

As a next step (can be a separate PR) it would make things much nicer to extract the results so they're visible in the GH UI (looks like it doesn't let viewing the artifacts, only downloading them). Something like this action:
https://github.com/marketplace/actions/publish-unit-test-results

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted. Makes sense to me.

verbose: true
flags: unit

- name: Archive unit test report
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have to archive?

Copy link
Member Author

Choose a reason for hiding this comment

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

username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}

- name: branch name tag
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this now as you moved the extraction of BRANCH_NAME in the Gradle plugin - https://github.com/hypertrace/hypertrace-gradle-docker-plugins/pull/21/files?


- name: Cache packages
id: cache-packages
uses: actions/cache@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the same version v2.3.4 everywhere.

Copy link
Contributor

@kotharironak kotharironak left a comment

Choose a reason for hiding this comment

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

I have a few minor comments, can you take care of it in follow up PR if you think they are valid.
LGTM

@JBAhire
Copy link
Member Author

JBAhire commented Dec 22, 2020

@kotharironak , will do that.

@JBAhire JBAhire merged commit 94a21c9 into main Dec 22, 2020
@JBAhire JBAhire deleted the gh-acction-cleanup branch December 22, 2020 04:41
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.

4 participants