Skip to content

Conversation

@JBAhire
Copy link
Member

@JBAhire JBAhire commented Jan 7, 2021

Description

Migrating to GitHub actions from CircleCI as per hypertrace/hypertrace#144

Testing

changes are updates as per discussions and workflow here: hypertrace/query-service#47

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld left a comment

Choose a reason for hiding this comment

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

Remove .DS_Store (and please update the .gitignore file) - this applies across many of these PRs

with:
args: build dockerBuildImages

validate-helm-charts:
Copy link
Contributor

Choose a reason for hiding this comment

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

This can move into the test workflow, no secrets are used requiring p_r_target

Copy link
Member Author

Choose a reason for hiding this comment

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

yes but there are no tests here so kept in the same file as we are keeping it in build and validate workflow at other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the workflows are divided by triggers though,

  • Would it make sense to merge the docker workflow into the other
  • In other places, I would move it too - p_r_target is a last resort for a couple different reasons (less secure, difficult to change since not workflow changes aren't testable without merging)

Copy link
Member Author

Choose a reason for hiding this comment

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

  • we shouldn't merge docker workflow into other as I will be moving that to pull_request separately.
  • I can create separate wokflow for that now and then I will merge that with e2e workflow when I will handle that.
  • or we can keep it as it is now and then handle with e2e solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you feel strongly, my preference would be to create the separate p_r based workflow now since that's a different issue than the e2e test one. If we want to leave the e2e as its own workflow pending the resolution around tags, that's fine.

Another thought about the e2e solution though - it's currently not doing a docker login before building the image, unlike in the build job. So...

  • Accept that it might get rate limited, unless we know of some reason why it wouldn't. In this case, we might as well remove the login from build and put that in a p_r triggered workflow too.
  • Keep it in a p_r_target workflow (and maybe just replace the build job since it appears to be a superset of it)
  • Migrate our base images to GHCR and not worry about rate limiting. This seems ideal, but is a separate piece of work most likely (but if its our go forward strategy, accepting the temporary rate limiting may be acceptable)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. will convert build workflow to pull_request and remove creds as this repo won't be getting lot of PRs anyways. Let's plan the base image migration to GHCR once this Github actions migration is completed

Copy link
Member Author

Choose a reason for hiding this comment

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

made similar change to all data service repos.

@JBAhire JBAhire merged commit 5c0284e into main Jan 7, 2021
@JBAhire JBAhire deleted the gha-ci branch January 7, 2021 14:59
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