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

chore: Changes to metadata files #20

Merged
merged 8 commits into from
Jan 23, 2024
Merged

Conversation

m-strzelczyk
Copy link
Collaborator

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:

  • Make sure to open an issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Follow the Samples Style Guide
  • Ensure the tests and linter pass
  • Communicate test infrastructure changes, i.e. API enablement, secrets
  • Appropriate docs were updated (if necessary)

🛠️ Fixes #<issue_number_goes_here>

@product-auto-label product-auto-label bot added size: s Pull request size is small. samples Issues that are directly related to samples. api: compute Issues related to the Compute Engine API. api: storage Issues related to the Cloud Storage API. labels Jan 2, 2024
@m-strzelczyk m-strzelczyk changed the title Changes to metadata files chore: Changes to metadata files Jan 2, 2024
Copy link

🤖 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 automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@m-strzelczyk m-strzelczyk force-pushed the strzelczyk/metadata-update branch from 8addb33 to e49360d Compare January 2, 2024 18:39
@m-strzelczyk m-strzelczyk marked this pull request as ready for review January 4, 2024 13:18
# 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."
Copy link
Collaborator

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?

Copy link
Collaborator Author

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"]
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jan 17, 2024
@m-strzelczyk m-strzelczyk merged commit f04eace into main Jan 23, 2024
5 checks passed
@m-strzelczyk m-strzelczyk deleted the strzelczyk/metadata-update branch January 23, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: compute Issues related to the Compute Engine API. api: storage Issues related to the Cloud Storage API. samples Issues that are directly related to samples. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants