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

Support for custom deploy user (#54) #55

Merged
merged 6 commits into from
Apr 20, 2021
Merged

Conversation

lukasbach
Copy link
Contributor

Adds the features requested in #54

Let me know if I should add or change anything. I'd be happy if the feature would be merged!

@lukasbach
Copy link
Contributor Author

@enriikke let me know if I can do something to get this PR merged. I'm happy to change or add anything you're unhappy with with the current changeset!

Copy link
Owner

@enriikke enriikke 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 fantastic @lukasbach! Thank you so much for taking the time to write up a PR and my apologies for taking long to reply. ✨ ❤️

I added some suggestions to your PR and also there are a couple of things I would like to see to get it merged:

  • Let's update the README to include the new inputs like we do for the existing inputs.
  • It's a bit tricky to write tests for CLI tools like this one but it would be great to include a few to make sure we call git config ... with the right thing based not he inputs.

index.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
@lukasbach
Copy link
Contributor Author

Hi @enriikke. Thanks for your suggestions, I've added them to the changeset. I've documented the inputs in the readme, added test cases for the arguments, removed the sha attribute from the commit message and formatted as you suggested. Feel free to let me know if there is something else I can do.

Also, I had issues using the nullish coalescing operator that you suggested (??), because the test case run in Github action complained that that's not valid Typescript. The project definitely uses a Typescript version that supports it, so I'm not sure if that's on jest-ts or something else maybe. Locally I did not get any error. Anyway, I've changed it to || because the variable can only either be a string or undefined anyways at this point.

Copy link
Owner

@enriikke enriikke 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 great! Thank you @lukasbach

@enriikke enriikke merged commit 380689c into enriikke:main Apr 20, 2021
@lukasbach
Copy link
Contributor Author

Cool, happy that I could help :)

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

2 participants