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

ci(): use GitHub actions instead of appveyor & travis #14

Merged
merged 11 commits into from
Sep 8, 2021
Merged

Conversation

smolck
Copy link
Collaborator

@smolck smolck commented Sep 8, 2021

ATM only have the publish workflow ported (next is the testing workflow to replace travis), which should publish to GitHub packages when run, although I need to test it (on my fork). And assuming we don't care about publishing to NuGet and GH packages, then no more dealing with the nuget API key expiring every year, so that's nice.

Some questions on this, I'm assuming we want to run the tests before doing the publishing, right? And when do we want to trigger the workflow? ATM I just have it set to run when triggered manually, but that should be able to be set up to run on pushing a version tag.

@smolck smolck added the ci related to automation for build, test, and release label Sep 8, 2021
@justinmk
Copy link
Member

justinmk commented Sep 8, 2021

Some questions on this, I'm assuming we want to run the tests before doing the publishing, right?

yes

And when do we want to trigger the workflow? ATM I just have it set to run when triggered manually, but that should be able to be set up to run on pushing a version tag.

Manual is fine for now. Let's just be sure to document the "release steps" similar to here. That makes the surface-area visible, and then we decide whether it makes sense to ever automate these steps.

Explicit is usually better, at least to start with. Implicit triggers like publish-on-tag can be surprising.

@smolck smolck marked this pull request as ready for review September 8, 2021 20:09
@smolck
Copy link
Collaborator Author

smolck commented Sep 8, 2021

Okay, this is ready for review; added the test workflow, the tests are run before the publishing part of the publish workflow, and got the publish workflow to publish a package finally after some trial and error.

Only thing left I guess is documentation on how to publish a package, which is pretty easy, just update the version file in src/NvimClient.API/NvimClient.API.csproj and run the workflow.

I can put the above (formatted in steps) at the bottom of the README, sound good? (Note that the last step is obviously only available for maintainers; I guess that could also be in the README maybe with the steps? not 100% sure . . .)

Also, I'll make a follow-up PR after this swapping out the CI status badges for the new GH actions ones (unless there's a way to do that without merging this first?).

@justinmk
Copy link
Member

justinmk commented Sep 8, 2021

Only thing left I guess is documentation on how to publish a package, which is pretty easy, just update the version file in src/NvimClient.API/NvimClient.API.csproj and run the workflow.

I can put the above (formatted in steps) at the bottom of the README, sound good?

yes

(Note that the last step is obviously only available for maintainers; I guess that could also be in the README maybe with the steps? not 100% sure . . .)

only maintainers can "release" anyway. Don't need to mention it.

README.md Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Show resolved Hide resolved

- name: Publish
run: |
dotnet nuget add source --username USERNAME --password ${{ secrets.GITHUB_TOKEN }} --store-password-in-clear-text --name github "https://nuget.pkg.github.com/neovim/index.json"
Copy link
Member

Choose a reason for hiding this comment

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

any idea why --store-password-in-clear-text is needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

README.md Outdated Show resolved Hide resolved
script:
- dotnet run --project
src/NvimClient.APIGenerator/NvimClient.APIGenerator.csproj
src/NvimClient.API/NvimAPI.generated.cs
Copy link
Member

Choose a reason for hiding this comment

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

the old script ran the generator, do we (plan to) have that in the GHA workflow?

even if it doesn't auto-create a PR or something, it's useful to exercise it I think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wondered about this; my thought for leaving it out is this way the file in the published package is the same as the one in the repo, so there aren't any unexpected discrepancies between the two.

Then we can just add another workflow that does update the API and create a PR if we want to automate that.

@smolck
Copy link
Collaborator Author

smolck commented Sep 8, 2021

Before I merge, thoughts on #14 (comment) ?

Also, just a note, once this is merged I'll wait to publish the package until I add the documentation for the public members it's giving warnings about (see here), so that the docs are in the release.

@smolck smolck merged commit d0a1554 into master Sep 8, 2021
@smolck smolck deleted the github-actions branch September 8, 2021 21:53
@kaashyapan kaashyapan mentioned this pull request Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci related to automation for build, test, and release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants