-
Notifications
You must be signed in to change notification settings - Fork 0
Add tags or labels to all resources created with shared modules #9
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
base: main
Are you sure you want to change the base?
Conversation
7d20313
to
b51d236
Compare
a5ebf9a
to
e83dc71
Compare
e83dc71
to
c590759
Compare
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.
Good idea!
- `main_file` - Python file name (e.g., "main.py"), relative to `source_dir`. | ||
- `schedule` - Cron expression (e.g., "0 9 * * 1-5") | ||
- `description` - Function/job description | ||
- `owner` - The owner/team responsible for this scheduled job |
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.
Nice! Do we want to use some consistent way of naming the team? Like should it be a github tag (@owner
or #team
), or the list of teams in ownership_data.json, or something else?
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.
Yeah, I should probably add a predefined list of teams here.
|
||
# Common tags for all resources | ||
locals { | ||
common_tags = merge(var.tags, { |
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 seems misnamed -- I'd expect common_tags
to just be the 3 common tags you list below, not that merged with var.tags
. Should it be all_tags
maybe?
Also, who wins in case of a merge conflict? I'm guessing that the ones below take precedence over the ones in var.tags
. Is that the desired behavior?
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 means "common for all resources generated by this usage of this module". Does "all_tags" sound better to you?
Good call on the merge conflicts, I'll make sure your guess is correct and document it.
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.
all_tags
sounds good to me!
``` | ||
|
||
### Supported Resources | ||
The following resources support tagging/labeling: |
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.
What happens if you try to add tags somewhere else? It seems like it's just silently ignored?
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.
If you add a field not supported in a terraform resource (in this case label
or tag
), it fails well before an apply
, I think it's during the check
stage.
Co-authored-by: Craig Silverstein <[email protected]>
Summary:
Added comprehensive resource tagging support to the scheduled-job Terraform module with automatic tags and custom tag support.
Changes Made
scheduled-job Module
tags
variable to allow custom taggingModule-Specific Tags
scheduled-job Module:
module
= "scheduled-job"job_name
= var.job_nameexecution_type
= var.execution_typeOther Modules
Resource Support
Legend:
Example Usage
Benefits
Backward Compatibility
tags
variable defaults to empty map{}
Additional Improvements
job_image
variable optional with proper validation for Cloud Run JobsIssue: INFRA-XXXX
Test plan:
Update culture-cron to use these updated modules, verify tags were created.