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

Action v0.0.1 #2

Merged
merged 43 commits into from
Oct 16, 2023
Merged

Action v0.0.1 #2

merged 43 commits into from
Oct 16, 2023

Conversation

kislerdm
Copy link
Contributor

@kislerdm kislerdm commented Oct 4, 2023

Resolves opentofu/opentofu#556

What changed

  • Added a port of the setup-terraform action. Note that the dependency hashicorp/js-releases was swapped with the adapter defined in lib/releases.js.

Why do we need it

  • To facilitate tofu adoption and integration into github CI pipelines

cc: @eranelbaz @cube2222

Copy link

@eranelbaz eranelbaz left a comment

Choose a reason for hiding this comment

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

I basically have a lot of questions
but I think it looks good

.github/workflows/setup-tofu.yml Outdated Show resolved Hide resolved
.github/workflows/setup-tofu.yml Show resolved Hide resolved
.github/workflows/setup-tofu.yml Outdated Show resolved Hide resolved
.github/workflows/setup-tofu.yml Outdated Show resolved Hide resolved
.github/workflows/setup-tofu.yml Outdated Show resolved Hide resolved
.github/workflows/setup-tofu.yml Outdated Show resolved Hide resolved
.github/workflows/setup-tofu.yml Show resolved Hide resolved
dist/index.js Show resolved Hide resolved
lib/setup-tofu.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
lib/setup-tofu.js Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
.github/workflows/data/local/main.tf Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/setup-tofu.yml Outdated Show resolved Hide resolved
.github/workflows/setup-tofu.yml Outdated Show resolved Hide resolved
.github/workflows/setup-tofu.yml Outdated Show resolved Hide resolved
Signed-off-by: Dmitry Kisler <[email protected]>
Copy link
Member

@Yantrio Yantrio left a comment

Choose a reason for hiding this comment

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

Overall it looks good, but I think this needs some cleaning up.

.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/setup-tofu.yml Outdated Show resolved Hide resolved
.github/workflows/setup-tofu.yml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
lib/releases.js Show resolved Hide resolved
lib/setup-tofu.js Outdated Show resolved Hide resolved
lib/setup-tofu.js Outdated Show resolved Hide resolved
lib/setup-tofu.js Outdated Show resolved Hide resolved
lib/setup-tofu.js Outdated Show resolved Hide resolved
@kislerdm
Copy link
Contributor Author

kislerdm commented Oct 12, 2023

@Yantrio Hey James! Thanks a lot for your review - I'll go through your comments thoroughly later today if you don't mind. A summarized reply to many of them: big part of the codebase was copied from setup-terraform without modification, hence the status quo may create some confusion and lead to the comments like "what year is it" 😀

I'll implement your suggestions, however we could also iteratively refactor after releasing v0.0.1 of the action so people could start using it already. For context, I intentionally did not invest effort into refactoring now to avoid postponing the action release.

WDYT?

@Yantrio
Copy link
Member

Yantrio commented Oct 12, 2023

@kislerdm Thanks for the quick reply and your efforts here!

I trust your judgement on my comments here to change what you think needs changing. Most of these are small nitpicks and can be easily fixed or changed.

for anything substantial requiring some refactoring and re-working, let's do it after in another PR. We can work together to make issues on things you want to do post-PR once you've had another go over my comments.

Thanks again! 🙌

@kislerdm
Copy link
Contributor Author

@Yantrio Hey James! Could you please have a look at the changes I made following your suggestions. Thanks!

@Yantrio
Copy link
Member

Yantrio commented Oct 13, 2023

I'm happy to approve and we can do more work in other issues/PRs later, I would just like these 2 comments addressing please :)

https://github.com/opentofu/setup-opentofu/pull/2/files#r1356483075
https://github.com/opentofu/setup-opentofu/pull/2/files#r1356477421

Thanks!

@kislerdm
Copy link
Contributor Author

@Yantrio Hey James, these comments have been addressed. Please let me know if there are more blockers which shall be tackled before the PR can be merged. Thanks!

@Yantrio Yantrio merged commit 6b016a1 into opentofu:main Oct 16, 2023
1 check passed
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.

Request: Support for GitHub Action
5 participants