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

resolves #168 migrate CI to GitHub Actions #169

Merged
merged 2 commits into from
Jan 19, 2021

Conversation

slonopotamus
Copy link
Contributor

No description provided.

@slonopotamus
Copy link
Contributor Author

Example run: https://github.com/slonopotamus/docker-asciidoctor/runs/1546816395?check_suite_focus=true

Obviously, you'll need to add DOCKERHUB_SOURCE_TOKEN and/or DOCKERHUB_TRIGGER_TOKEN to GitHub Actions secrets in order to make deployment work on https://github.com/asciidoctor/docker-asciidoctor/settings/secrets/actions

@slonopotamus
Copy link
Contributor Author

Also, current repository settings require Travis build to pass successfully before PR can be merged. This cannot be satisfied anymore because I'm removing Travis build file :)

Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

Thanks for this great work, of for your patience before having a review!

Some feedbacks have been added if it is ok for you?

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

LGTM great job!

Approving this as my last comment is a nice to have (and not blocking the PR in any way).

@dduportal
Copy link
Contributor

  • The Travis CI check had been removed
  • The 2 secrets have been added

=> I'm adding an empty commit to this PR to trigger a first GH action build (as it seems to require a repository's collaborator to start)

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal dduportal merged commit 9d7afad into asciidoctor:master Jan 19, 2021
@slonopotamus
Copy link
Contributor Author

Note that I did not test that upload to docker actually works, though I don't see any reasons why it shouldn't after secrets are added.

@slonopotamus
Copy link
Contributor Author

slonopotamus commented Jan 19, 2021

You can't test upload in a PR because PRs do not have access to repo secrets neither on Travis nor on GitHub Actions, this is a security measure.

@dduportal
Copy link
Contributor

No problem @slonopotamus! Thank you for this excellent work. I've merged the PR to allow testing (and first gh action setup) on master branch to benefit from the secrets.

@dduportal
Copy link
Contributor

You can't test upload in a PR because PRs do not have access to repo secrets neither on Travis nor on GitHub Actions, this is a security measure.
Our messages crossed, thanks a lot for this precision!

@slonopotamus slonopotamus deleted the gh-actions branch January 19, 2021 09:14
@slonopotamus
Copy link
Contributor Author

slonopotamus commented Jan 19, 2021

Hmm... Deploy cannot access secrets... I guess we need to expose secrets via environment variables to Makefile. A couple of minutes, will create a follow-up PR.

@slonopotamus
Copy link
Contributor Author

I think #171 should do the trick.

@dduportal
Copy link
Contributor

@slonopotamus yes, you might want to define an env: directive in the deploy section as underlined in https://docs.github.com/en/actions/reference/encrypted-secrets#using-encrypted-secrets-in-a-workflow to expose the secret to environment

@slonopotamus
Copy link
Contributor Author

We could also declare it a Makefile bug: Makefile doesn't return non-zero exit code when deploy fails.

@dduportal
Copy link
Contributor

We could also declare it a Makefile bug: Makefile doesn't return non-zero exit code when deploy fails.

Yes, totally! And I might be the culprit for that 😅

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