-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore: Changes to metadata files #20
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
8addb33
to
e49360d
Compare
# What will happen when the code sample in the region tag gets executed. Natural Language. (optional) | ||
effects = "The template gets deleted." | ||
effects_desc = "The template gets deleted." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is _desc meant to clarify that it's a string value? Are these values separately used in the tool? If not maybe they should be combined? Or perhaps requirements_desc could be moved as requirements.description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _desc suffix is there, because right after this we have a section [requirements]
and the names collide. So I quickly added the _desc suffix to requirements fields. I added the suffix to effects as well for consistency.
# execute some commands before `terraform destroy` takes place. | ||
commands = ["gsutil rm -f gs://$BUCKET_NAME/$NEW_FILE_NAME"] | ||
|
||
commands = ["gsutil rm -f gs://$BUCKET_NAME/$NEW_FILE_NAME || true"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense for prototype, it might be good to move the error suppression into a separate setting, maybe a "warnOnFailure" that would allow the tool to suppress the error but still log as part of the tool's UX that something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could work when we have only one command, but if there are multiple commands and we want to ignore error for only some of them, it gets more complicated.
Definitely something to consider if this gets to evolve to full project.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
🛠️ Fixes #<issue_number_goes_here>