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

Add an action to publish the grammar automatically #98

Merged
merged 3 commits into from
Sep 2, 2022

Conversation

Luni-4
Copy link
Contributor

@Luni-4 Luni-4 commented Jan 14, 2022

Checklist:

  • 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! :)

@aryx aryx requested a review from dcreager January 15, 2022 20:01
@aryx
Copy link
Contributor

aryx commented Mar 9, 2022

@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?

@Luni-4
Copy link
Contributor Author

Luni-4 commented Mar 9, 2022

@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
Copy link
Contributor Author

Luni-4 commented Mar 19, 2022

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

@aryx
Copy link
Contributor

aryx commented Mar 21, 2022

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

@Luni-4
Copy link
Contributor Author

Luni-4 commented Mar 21, 2022

@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

@aryx
Copy link
Contributor

aryx commented Mar 21, 2022

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.

@Luni-4
Copy link
Contributor Author

Luni-4 commented Mar 21, 2022

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

@aryx
Copy link
Contributor

aryx commented Mar 21, 2022

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

@aryx
Copy link
Contributor

aryx commented Mar 21, 2022

cc @patrickt

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld commented Mar 21, 2022

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.

@Luni-4
Copy link
Contributor Author

Luni-4 commented Mar 22, 2022

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
Copy link
Contributor Author

Luni-4 commented Apr 14, 2022

@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!

@maxbrunsfeld
Copy link
Contributor

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

@maxbrunsfeld maxbrunsfeld merged commit 81148c0 into tree-sitter:master Sep 2, 2022
@Luni-4
Copy link
Contributor Author

Luni-4 commented Sep 5, 2022

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
Copy link
Contributor Author

Luni-4 commented Sep 16, 2022

@Luni-4
Copy link
Contributor Author

Luni-4 commented Dec 2, 2022

@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!

@aryx
Copy link
Contributor

aryx commented Dec 6, 2022

@Luni-4
Copy link
Contributor Author

Luni-4 commented Dec 6, 2022

Wow, it worked perfectly! Thank you @aryx!

When tree-sitter/tree-sitter#1859 (comment) will be ready, I'm going to add the action to every grammar available for tree-sitter

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.

None yet

3 participants