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

Initial build of action #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

webstech
Copy link
Collaborator

This action supports a number of commands and would be part of scheduled GitHub workflows to perform the commands. These actions are supporting gitgitgadget functions that allow email patch based repos to work with GitHub.

All input is welcome. The doc probably suffers from author familiarity issues. The README is generated from the actions.yml using a randomly selected tool. If there are known alternatives, let me know.

@webstech webstech requested a review from dscho September 20, 2022 06:17
Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

This is a great start!

Just to make sure: you are planning on using something like https://github.com/marketplace/actions/action-github-app-token to generate the token in the workflow, right? I wonder whether it wouldn't make sense to fold that functionality into the Action, given that gitgitgadget already contains code to do that.

Speaking of the Action, how about renaming it from check-status to gitgitgadget-action? That would help with forks (speaking from experience, people who fork git-for-windows/build-extra hate that their fork's name does not have git-for-windows in its name, as they are prone to forget what that particular repository is all about after a couple of years).

LICENSE Outdated
@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2019 Peter Evans
Copy link
Member

Choose a reason for hiding this comment

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

Peter Evans? 😁

Oh, and 2019. Wasn't that looong, looooong time ago, like a life time ago?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please point to a preferred licence.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with MIT. Or with ISC. Just change the name and the year ;-)

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
package.json Outdated
"eslint": "^8.23.1",
"eslint-plugin-anti-trojan-source": "^1.1.0",
"eslint-plugin-github": "^4.3.7",
"gitgitgadget": "file:../gitgitgadget/gitgitgadget-1.0.0.tgz",
Copy link
Member

Choose a reason for hiding this comment

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

How about specifying it directly, i.e. as a link to the repository?

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 was avoiding checking in the package as part of gitgitgadget. Assumed there would be very little need for it. The instructions identify how to create it. It can be added to ggg if that is preferred. The package could also be published to npm, etc. if that is a preferred direction. Trying to limit the spread on something with limited use.

OTOH, did I misunderstand your suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

I thought that we could simply refer to the gitgitgadget/gitgitgadget repository without changing anything in that repository, is that not the case? Would we have to commit transpiled code?

Copy link
Member

Choose a reason for hiding this comment

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

According to https://docs.npmjs.com/cli/v8/configuring-npm/package-json#git-urls-as-dependencies:

When installing from a git repository, the presence of certain fields in the package.json will cause npm to believe it needs to perform a build.
[...]
build
[...]
install

I therefore think that it should simply work, no changes in gitgitgadget/gitgitgadget required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is needed to build the action are the gitgitgadget lib source files. The npm pack makes them available with the deps info. Having npm run a build is not going to make those files available as far as I can tell (no, I did not run a test). The npm build feature seems like a way to make tools available that have not been published to a package manager.

Copy link
Member

Choose a reason for hiding this comment

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

The npm build feature seems like a way to make tools available that have not been published to a package manager.

Yes, and I think that is our use case. I should have a little bit of time to play with this next week.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, and I think that is our use case.

Well, sort of. From the doc it appears npm will do a build that we don't need. It then does the pack (or equivalent) and installs that. I was concerned it would try to use the build output but someone (okay, me) added files to the package.json and it all works. I will update the package.json and README.md to show this simpler process.

Thanks for catching this and I guess I should have run that test.

test/check-status.cmd Show resolved Hide resolved
@webstech
Copy link
Collaborator Author

Just to make sure: you are planning on using something like https://github.com/marketplace/actions/action-github-app-token to generate the token in the workflow, right? I wonder whether it wouldn't make sense to fold that functionality into the Action, given that gitgitgadget already contains code to do that.

That looks interesting but doing it internally is probably better. Non-gitgitgadget projects may not have an app_id so they will also need to be supported. I will look into this some more. It may mean more optional parameters.

Speaking of the Action, how about renaming it from check-status to gitgitgadget-action? That would help with forks (speaking from experience, people who fork git-for-windows/build-extra hate that their fork's name does not have git-for-windows in its name, as they are prone to forget what that particular repository is all about after a couple of years).

How about gitgitgadget-action-check-status? gitgitgadget-action-handle-pr is in progress.

@webstech
Copy link
Collaborator Author

@dscho Thanks for the prompt review. This is one piece of many.

@dscho
Copy link
Member

dscho commented Sep 21, 2022

Speaking of the Action, how about renaming it from check-status to gitgitgadget-action? [...]

How about gitgitgadget-action-check-status?

Why check-status? According to action.yml this Action supports many modes, none of which really fit into the description of "checking a status". I think I really like gitgitgadget-action better...

@webstech
Copy link
Collaborator Author

Why check-status? According to action.yml this Action supports many modes, none of which really fit into the description of "checking a status". I think I really like gitgitgadget-action better...

I thought the actions were checking the status of the notes vs reality and the status of email tracking. It may be a simplistic view (it could easily have been update-status). The problem I have with gitgitgadget-action is that there are multiple actions (handling PRs being another, and maybe auth as well). With that in mind, I am open to other suggestions. In the end, we either remember the name or scowl and check the readme. 🙁

@webstech
Copy link
Collaborator Author

That looks interesting but doing it internally is probably better. Non-gitgitgadget projects may not have an app_id so they will also need to be supported. I will look into this some more. It may mean more optional parameters.

Forgot I had done a similar implementation in the package branch of my ggg repo. Guess I had a good vacation! Either way, a token of some sort is needed (to generate a token or the real token). Maybe both can be supported - testing is easier without needing to have the app.

@dscho
Copy link
Member

dscho commented Sep 22, 2022

Guess I had a good vacation!

😁

Either way, a token of some sort is needed (to generate a token or the real token). Maybe both can be supported - testing is easier without needing to have the app.

That makes sense.

Signed-off-by: Chris. Webster <[email protected]>
@dscho
Copy link
Member

dscho commented Jan 14, 2023

Hey @webstech! After mulling over this for quite a few months, I think it would make most sense to fold this back into gitgitgadget/gitgitgadget with the following idea: make it a single GitHub Action (using ncc and such, as you did) that takes only two (or three) parameters: one to point to a file that contains the configuration as JSON, and another parameter to essentially contain the command-line that is currently passed to the misc-helper.ts script (the third parameter would contain the secret necessary to act as the GitHub App(s), but it is probably a better idea to implement that via environment variables).

That would solve a lot of headache I had about maintaining several Actions and the ensuing dependency hell.

What do you think?

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