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

Start migrating CircleCI workflow to GitHub actions #3432

Merged
merged 1 commit into from
Jun 8, 2021
Merged

Start migrating CircleCI workflow to GitHub actions #3432

merged 1 commit into from
Jun 8, 2021

Conversation

owais
Copy link
Contributor

@owais owais commented May 19, 2021

Introduce GitHub Actions build/test/publish workflow. This ports most of the build-publish workflow from CircleCI to Github Actions. Most of the pipeline and the scripts have been taken from the core repo and adapted where necessary to work with contrib.

Pending work items (will be completed in separate PRs):

  • Enable publish-dev and publish-stable jobs (disbaled for now; need to setup secrets with proper protection).
  • Integrate github issuegenerator to report some failures such as loadtest as github issues.
  • Disable CircleCI.

Regressions

Duplication in Github YAML (separate work item):

Github does not support re-usable steps, running actions from within other actions or YAML anchors like CircleCI. As a result there is a lot of duplication in the file. I'll follow up this with a solution to elimination the duplication. I'm thinking to have a build-and-test.in.yaml file which will use anchors to re-use blocks (like go and tooling cache, publish stable+dev, etc) and then have a tool that generates another build-and-test.yaml from that file with expanded anchors. This is a separate work item though so not included in this PR.

Can't re-run only failed jobs

This can be extremely annoying. If a single job fails in a workflow, there is no way to retry just the failed job. Only way to retry is to retry the entire workflow. This has been very annoying for personally with other Otel and Splunk projects. GitHub has acknowledged that the feature is on their roadmap but there is no ETA on this. We can hack together a solution that stores the result (pass/fail) of every job in workflow somewhere and on re-runs skips jobs if:

  • they passed in the previous run of the same workflow
  • no files affecting the job had any changes

This is very much possible to implement but not sure if it is worth it. Either way we can invest time into it IF it turns out to be a big issue.

Pipelines

CircleCI

image

Github Actions

image

GitHub workflow lists all jobs even if they're not supposed to run in that workflow such as publish-dev/publish-stable in ^ screenhot.

@owais owais changed the title Move jobs to GitHub actions Start migrating CircleCI workflow to GitHub actions May 21, 2021
@owais owais marked this pull request as ready for review May 21, 2021 08:26
@owais owais requested a review from a team May 21, 2021 08:26
@owais owais marked this pull request as draft May 21, 2021 09:48
@mx-psi
Copy link
Member

mx-psi commented May 21, 2021

One item that IMO we should add to the pending work items list is to enable Dependabot for Github Actions. Maybe we should have a separate issue to track this?

@owais
Copy link
Contributor Author

owais commented May 21, 2021

Yes, that doesn't seem related to CI jobs. Separate issue would be better IMO.

@tigrannajaryan
Copy link
Member

I'm thinking to have a build-and-test.in.yaml file which will use anchors to re-use blocks (like go and tooling cache, publish stable+dev, etc) and then have a tool that generates another build-and-test.yaml from that file with expanded anchors. This is a separate work item though so not included in this PR.

SGTM.

Can't re-run only failed jobs

A good reason for us to focus on making our tests rock stable (the tests are the offenders which require rerunning). Let's spend the time we would spend on finding a solution to rerun single jobs on making our tests stable. It is a better investment IMO.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

The approach LGTM, thanks for working on this.

@owais owais marked this pull request as ready for review May 21, 2021 15:38
@owais
Copy link
Contributor Author

owais commented May 21, 2021

A good reason for us to focus on making our tests rock stable (the tests are the offenders which require rerunning). Let's spend the time we would spend on finding a solution to rerun single jobs on making our tests stable. It is a better investment IMO.

Definitely but despite that this can be an annoyance for people working on new features and using CI for tests. Luckily I think collector's local workflow is simple enough to run and verify everything locally so CI shouldn't be required to be part of the local iteration process.

@tigrannajaryan
Copy link
Member

@owais can you please rebase, the failed loadtest should pass now.

publish-stable:
runs-on: ubuntu-latest
needs: [lint, unittest, integration-tests, build-package, windows-msi]
if: ${{ false }}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to explain why this is excluded?

runs-on: ubuntu-latest
needs: [lint, unittest, integration-tests, build-package, windows-msi]
if: ${{ false }}
# if: startsWith(github.ref, 'refs/tags/') && github.repository == 'open-telemetry/opentelemetry-collector-contrib'
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? Can we delete commented code or turn it into a TODO comment?

runs-on: ubuntu-latest
needs: [lint, unittest, integration-tests, build-package, windows-msi]
if: ${{ false }}
# if: (github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/')) && github.repository == 'open-telemetry/opentelemetry-collector-contrib'
Copy link
Member

Choose a reason for hiding this comment

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

Same here: Can we delete commented code or turn it into a TODO comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment left, will merge after that is addressed.

@owais owais requested a review from tigrannajaryan May 31, 2021 16:17
@owais
Copy link
Contributor Author

owais commented May 31, 2021

Looks like loadtests are unreliable on GH actions compared to Circle. Nothing blocks this PR but we might want to keep loadtests on Circle for a while. In the long run, we probably need funding to be able to run loadtests in the cloud where we can predict performance a bit more reliably.

Copy link
Member

@gramidt gramidt left a comment

Choose a reason for hiding this comment

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

There appears to be a new empty file 'otelcontribcol-all-sys' as part of this PR. Was this intentional?

@owais
Copy link
Contributor Author

owais commented Jun 1, 2021

@gramidt No, removed. Thanks

@tigrannajaryan
Copy link
Member

@bogdandrutu this looks good to me. We can merge if you don't have any additional thoughts.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

This looks good to me as a high-level direction, did not look at the code but trust @tigrannajaryan

@@ -73,3 +77,4 @@ function Confirm-MSI {

$sb = [scriptblock]::create("$Target")
Invoke-Command -ScriptBlock $sb
exit 0
Copy link
Member

Choose a reason for hiding this comment

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

New line.

This ports most of the build-publish workflow from CircleCI to Github
Actions.

Pending work items:

- Enable publish-dev and publish-stable jobs (disbaled for now; need to
setup secrets with proper protection).
- Integrate github issuegenerator to report some failures such as
loadtest as github issues.
- Disable CircleCI.
@bogdandrutu bogdandrutu merged commit 2378271 into open-telemetry:main Jun 8, 2021
@owais owais deleted the move-jobs-to-github-actions branch June 8, 2021 11:06
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
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.

6 participants