tree-sitter-java icon indicating copy to clipboard operation
tree-sitter-java copied to clipboard

Published 20 hours ago • verified publisher icon tree-sitter

Add an action to publish the grammar automatically

Open Luni-4 opened this issue 3 years ago • 12 comments

Checklist:

  • [x] All tests pass in CI.
  • [ ] There are sufficient tests for the new fix/feature.
  • [ ] Grammar rules have not been renamed unless absolutely necessary.
  • [ ] The conflicts section hasn't grown too much.
  • [ ] The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).

This PR allows to publish automatically tree-sitter-java on crates.io when a new tag is set. To make it work, it is just necessary to define the CARGO_REGISTRY_TOKEN as secret.

Thanks in advance for your review! :)

Luni-4 avatar Jan 14 '22 12:01 Luni-4

@Luni-4 could you rebase on the latest version? Is there another example of such a workflow in one of the other tree-sitter-xxx repositories?

aryx avatar Mar 09 '22 13:03 aryx

@Luni-4 could you rebase on the latest version? Is there another example of such a workflow in one of the other tree-sitter-xxx repositories?

Done. Nope, there isn't, but I can add this workflow in another tree-sitter-xxx repository if you want, no problem

Luni-4 avatar Mar 09 '22 16:03 Luni-4

Can we merge this PR @dcreager? Or does it need to be approved again?

Luni-4 avatar Mar 19 '22 16:03 Luni-4

what about the CI failure on windows? Any idea @Luni-4 ?

aryx avatar Mar 21 '22 07:03 aryx

@aryx I think it's not related to this PR because it doesn't change anything of the code structure. Looking at it it seems it doesn't find a VS installation on the image https://github.com/tree-sitter/tree-sitter-java/runs/5486901153?check_suite_focus=true#step:4:32

Luni-4 avatar Mar 21 '22 08:03 Luni-4

I don't have the permissions to merge without CI passing. I've rerun the job in case it's due to a flakyness but otherwise @dcreager or @maxbrunsfeld needs to force merge it.

aryx avatar Mar 21 '22 08:03 aryx

@aryx Now we can merge this one, please add the CARGO_REGISTRY_TOKEN too, thanks!

Luni-4 avatar Mar 21 '22 09:03 Luni-4

I don't have the permission to set secret tokens on this repo. cc @dcreager @maxbrunsfeld

aryx avatar Mar 21 '22 10:03 aryx

cc @patrickt

aryx avatar Mar 21 '22 10:03 aryx

Sorry for dropping this thread @Luni-4. I think this is a very good change.

Looking at the GH actions configuration settings pages, there are a few decisions to make about "Environment" in which this secret is active. For example, what branches it should be enabled for, and protections on those branches. I can make a decision on that, but I don't have super strong feelings on the security aspects of it.

I also have a little bit less time to work on these in the past few months, so I'm wondering if some other stakeholder in these repos can oversee this. @dcreager @rewinfrey @patrickt I'm wondering if your team at GitHub has the cycles to push this effort over the finish line. AFAICT, it entails:

  • Configuring the GH Actions environment that will store this crates.io registry token:
    • Should it just be main/master? As a separate question, should we rename any master default branches to main in order to abide by the more recent convention?
  • Adding the actual API token to the environment.
    • Whose API token should be used to publish to crates.io? I'm ok having it be mine, but I'd also be fine if someone at GH wants to set it up using their own crates.io account.
  • Actually running a tagged build and verifying that the right thing is published, to make sure that everything is configured correctly.
  • Setting up something similar to this PR for the other grammars in the tree-sitter org.

maxbrunsfeld avatar Mar 21 '22 20:03 maxbrunsfeld

Sorry for dropping this thread @Luni-4. I think this is a very good change.

No problem! Thanks!

Looking at the GH actions configuration settings pages, there are a few decisions to make about "Environment" in which this secret is active. For example, what branches it should be enabled for, and protections on those branches. I can make a decision on that, but I don't have super strong feelings on the security aspects of it.

The secret should be assigned to the repository, so if another fork tries to create a tagged version, the GH action doesn't publish anything because the API token is not present in the forked repository.

* Adding the actual API token to the environment.
  
  * Whose API token should be used to publish to crates.io? I'm ok having it be mine, but I'd also be fine if someone at GH wants to set it up using their own crates.io account.

We can create a GH account called tree-sitter-publisher for this organization and add this new account to each tree-sitter-crate on crates.io. In this way the publishing is not associated to a single person but anyone who manages that account.

* Actually running a tagged build and verifying that the right thing is published, to make sure that everything is configured correctly.

We can try with the 0.20 version of tree-sitter-java. As the action is structured, the code will not be published immediately, indeed a dry-run check is run before to test that everything is configured correctly.

* Setting up something similar to this PR for the other grammars in the `tree-sitter` org.

I'm willing to add this action to every other grammar in this org, unfortunately I will not be able to setup secrets.

Luni-4 avatar Mar 22 '22 08:03 Luni-4

@maxbrunsfeld Can we merge this one please? We need tree-sitter-java 0.20 to release our software and we are blocked by this PR, thanks!

Luni-4 avatar Apr 14 '22 09:04 Luni-4

Ok, I've added that CARGO_REGISTRY_TOKEN secret environment variable at the Tree-sitter organization level. Let's see how this goes.

maxbrunsfeld avatar Sep 02 '22 22:09 maxbrunsfeld

Ok, I've added that CARGO_REGISTRY_TOKEN secret environment variable at the Tree-sitter organization level. Let's see how this goes.

Thank you! Can we bump a 0.20.1 version @aryx and @maxbrunsfeld and see if it works?

Luni-4 avatar Sep 05 '22 06:09 Luni-4

Just a feedback: the action works on https://github.com/tree-sitter/tree-sitter-c-sharp, here the outcome https://github.com/tree-sitter/tree-sitter-c-sharp/actions/runs/3058879080/jobs/4935595332

Luni-4 avatar Sep 16 '22 08:09 Luni-4

@aryx @maxbrunsfeld Can we release a new 0.20.1 version? It would be helpful to update and release rust-code-analysis and at the same time we can verify if the action works. Thank you!

Luni-4 avatar Dec 02 '22 08:12 Luni-4

Just did here: https://github.com/tree-sitter/tree-sitter-java/releases/tag/v0.20.1

aryx avatar Dec 06 '22 09:12 aryx

Wow, it worked perfectly! Thank you @aryx!

When https://github.com/tree-sitter/tree-sitter/pull/1859#issuecomment-1333841459 will be ready, I'm going to add the action to every grammar available for tree-sitter

Luni-4 avatar Dec 06 '22 09:12 Luni-4