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

Port CI to GitHub Actions #32

Merged
merged 1 commit into from
Aug 24, 2020
Merged

Port CI to GitHub Actions #32

merged 1 commit into from
Aug 24, 2020

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Jul 31, 2020

This should emulate the behavior of the previous Azure Pipeline CI build. The only exception is the build artifact is
now stored as a GitHub Actions workflow artifact rather than on Azure Pipeline.

To work around the actions/upload-artifact action's behavior of removing executable file permissions from the workflow
artifacts (actions/upload-artifact#38), I uploaded the archive file as the artifact. The downside to this workaround is you get a .zip file inside of
a .zip file:

binary_Linux.zip
|_ arduino-language-server_Linux_amd64.zip
    |_ arduino-language-server

This should emulate the behavior of the previous Azure Pipeline CI build. The only exception is the build artifact is
now stored as a GitHub Actions workflow artifact rather than on Azure Pipeline.

To work around the actions/upload-artifact action's behavior of removing executable file permissions from the workflow
artifacts, I uploaded the archive file as the artifact. The downside to this workaround is you get a .zip file inside of
a .zip file:

binary_Linux.zip
|_ arduino-language-server_Linux_amd64.zip
    |_ arduino-language-server
@per1234
Copy link
Contributor Author

per1234 commented Jul 31, 2020

@rsora this will require the creation of the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY repository secrets. Can you create those for us?

@per1234 per1234 requested a review from kittaakos July 31, 2020 07:53
@kittaakos
Copy link
Contributor

he downside to this workaround is you get a .zip file inside of
a .zip file:

That's not a problem for the Pro IDE. It uses the s3 download link.


- name: Publish Nightly [S3]
if: github.event_name == 'schedule` || github.event_name == 'workflow_dispatch'
uses: kittaakos/upload-s3-action@v0.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe It's time to check if the guy merged the fix proposed by Akos in the upstream repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still open: shallwefootball/upload-s3-action#10
It looks like the PR has some requested changes.

I see there has been a release in the upstream repo since the point we're using in the fork. If I understand correctly, the benefit of using the kittaakos/upload-s3-action action instead of upstream is correct error handling, so, if everything is working correctly, it should be possible to use the upstream instead if you prefer. On the other hand, we won't know whether it's working correctly until after this PR is merged, since this publish step is the only part I didn't test.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still open: shallwefootball/upload-s3-action#10
It looks like the PR has some requested changes.

I will do an update.

On the other hand, we won't know whether it's working correctly until after this PR is merged, since this publish step is the only part I didn't test.

+1 for sticking to the forked action in the PR.

@rsora
Copy link
Contributor

rsora commented Jul 31, 2020

@rsora this will require the creation of the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY repository secrets. Can you create those for us?

Done!

@per1234
Copy link
Contributor Author

per1234 commented Jul 31, 2020

That's not a problem for the Pro IDE. It uses the s3 download link.

My understanding is that the sole purpose for the step publishing the pipeline artifact in this repository's current Azure Pipeline configuration is facilitating beta testing. Is that correct @kittaakos ?
https://github.com/bcmi-labs/arduino-language-server/blob/e8103c5a469a2cc4de740abf1bad29dddeaef0bd/azure-pipelines.yml#L52-L53

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

is facilitating beta testing.

FYI: It's always easier to build the LS locally and try it out immediately with the Pro IDE.

For me, the workflow should be something like this:

  • build on all platforms then use actions/upload,
  • use actions/download on Linux only,
  • and do the s3 upload only when all platform builds were green.

I agree with @per1234's sugggestion:

I'd say that particular improvement would be better as a separate PR to keep this PR about a straight across port.

Let's keep everything as-is for now.

I am going to try the LS in action today. Please keep this PR open until we have the 0.0.7 Pro IDE release.


- name: Publish Nightly [S3]
if: github.event_name == 'schedule` || github.event_name == 'workflow_dispatch'
uses: kittaakos/upload-s3-action@v0.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

It's still open: shallwefootball/upload-s3-action#10
It looks like the PR has some requested changes.

I will do an update.

On the other hand, we won't know whether it's working correctly until after this PR is merged, since this publish step is the only part I didn't test.

+1 for sticking to the forked action in the PR.

@kittaakos
Copy link
Contributor

I am going to merge this change. If something does not we can still do a revert.

Thank you, @per1234 👍

@kittaakos kittaakos merged commit 8ecee04 into arduino:master Aug 24, 2020
@per1234 per1234 deleted the port-ci branch August 24, 2020 12:20
@per1234 per1234 self-assigned this Nov 23, 2021
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