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

Refactor build_and_tests GHA workflow before for migrating publish jobs #2313

Merged

Conversation

shovnik
Copy link
Contributor

@shovnik shovnik commented Dec 21, 2020

Which problem is this solving?

As part of issue, this pull request does some naming changes and refactoring on the already migrated build_and_test workflows jobs in preparation for migrating the remaining windows-msi, publish-dev and publish-stable jobs in the following PR
This PR must be reviewed and merged before merging the final migration PR. The purpose of this PR is to remove changes to existing jobs from the final migration PR.

What changes are made in this PR?

  • All step names are consistent across jobs (descriptive and capitalized except articles/prepositions)
  • Bash logic that is complex or to be reused in the new jobs was extracted into shell scripts
  • Permissions were added to binaries before upload to allows windows machines to execute them. This meant archiving the binaries with tar before upload for them to retain permissions.
    cc- @alolita, @AzfaarQureshi

@shovnik shovnik requested a review from a team December 21, 2020 16:33
@shovnik shovnik changed the title Refactoring build_and_tests GitHub Actions workflow in preparation for migrating publish jobs Refactoring build_and_tests GHA workflow in preparation for migrating publish jobs Dec 21, 2020
@codecov
Copy link

codecov bot commented Dec 21, 2020

Codecov Report

Merging #2313 (80d9700) into master (d47baee) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2313   +/-   ##
=======================================
  Coverage   92.04%   92.04%           
=======================================
  Files         272      272           
  Lines       15337    15337           
=======================================
  Hits        14117    14117           
  Misses        840      840           
  Partials      380      380           

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 d47baee...80d9700. Read the comment docs.

@shovnik
Copy link
Contributor Author

shovnik commented Dec 21, 2020

@bogdandrutu I separated out the naming changes and refactoring to the existing workflow into a separate PR as requested. Please take a look.

Comment on lines 77 to 78
- name: Create Tool Binaries Archive
run: tar -cvf tool-bin.tar /home/runner/go/bin
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed vs the whole path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I archived the file was to preserve the execute permissions on the binaries created. Specifically on the windows-msi job, there was no way to easily chmod +x the binary once downloaded given the OS. Also, this prevents having to add permissions to the binaries everytime it is downloaded although extracting the archive is required instead.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine to always add the chmod +x instead of archive. Maybe leave also a TODO to explain this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only issue is that I cannot do the chmod for the windows job (In PR 3/3), this was a workaround specifically for that. I can remove the archiving from tool binaries, but it is needed for the collector binaries which the windows-msi job uses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the archiving from all the tool binaries, because it was unnecessary. I still need to archive the collector binary produced from cross-compile because it requires execute permissions to run in windows-msi.

Copy link
Member

Choose a reason for hiding this comment

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

It does not seem to be reflected in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed my amend to the wrong Git branch, did not mean to mislead sorry. All instances of tool archive should be gone now.

@shovnik shovnik changed the title Refactoring build_and_tests GHA workflow in preparation for migrating publish jobs Refactor build_and_tests GHA workflow in preparation for migrating publish jobs Dec 21, 2020
@shovnik shovnik changed the title Refactor build_and_tests GHA workflow in preparation for migrating publish jobs Refactor build_and_tests GHA workflow before for migrating publish jobs Dec 21, 2020
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu merged commit bb64e04 into open-telemetry:master Dec 22, 2020
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
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.

2 participants