-
Notifications
You must be signed in to change notification settings - Fork 19
feat: Adding Submodule for Standalone Workflow for ADC #54
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
|
Requesting to review this PR. |
a5a507c to
9180d2a
Compare
a56480a to
3b177bf
Compare
- Added Submodule for Standalone workflow - Added Validations and metadata.display.yaml file for making submodule ADC compliant - Added relevant examples and test
3b177bf to
aecc2c0
Compare
|
/gcbrun |
| type = string | ||
| } | ||
|
|
||
| variable "workflow_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.
I think it should be marked as level 1 as it doesn't seem an advanced field.
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.
done
modules/simple_workflow/variables.tf
Outdated
| variable "workflow_description" { | ||
| description = "Description for the cloud workflow" | ||
| type = string | ||
| default = "Sample workflow 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.
Update the default value to null.
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.
done
modules/simple_workflow/variables.tf
Outdated
| variable "service_account_email" { | ||
| description = "Service account email. Unused if service account is auto-created." | ||
| type = string | ||
| default = null | ||
| } | ||
|
|
||
| variable "service_account_create" { | ||
| description = "Auto-create service account." | ||
| type = bool | ||
| default = false | ||
| } |
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.
I think one of this at least should be a level1 field.
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.
Removed service account create option itself, we will be using the SA component in ADC itself
examples/simple_workflow/main.tf
Outdated
| workflow_name = "standalone-workflow" | ||
| region = "us-central1" | ||
| service_account_email = module.service_account.email | ||
| service_account_create = 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.
I think this test/example should be renamed to something like with_service_account_auto_create and we should add more test to cover other scenarios related to service_account input.
Tests can be added later also.
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.
as we have removed autocreate SA logic, this will be not required
| dir: /modules/simple-workflow | ||
| ui: | ||
| input: | ||
| variables: |
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.
Please check if we want to mark any optional input as level 1 if we want to show it along the required values in the ADC UI.
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.
added optional input as level1
52c745e to
fb37486
Compare
fb37486 to
0a5caf0
Compare
ceaa610 to
e14c5fe
Compare
anaik91
left a comment
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.
lgtm
| * limitations under the License. | ||
| */ | ||
|
|
||
| terraform { |
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.
We can omit this for examples
examples/simple_workflow/outputs.tf
Outdated
| output "project_id" { | ||
| description = "Google Cloud project in which the workflow is deployed" | ||
| value = var.project_id | ||
| } |
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 this is for use in test, lets use GetTFSetupStringOutput("project_id") to retrieve it rather than redirecting inputs as outputs
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 this is for use in test, lets use
GetTFSetupStringOutput("project_id")to retrieve it rather than redirecting inputs as outputs
updated to use GetTFSetupStringOutput
249f4c3 to
798148c
Compare
|
/gcbrun |
798148c to
e8e8f2e
Compare
|
/gcbrun |
981ee1e to
d4e1f27
Compare
82cff5a to
07bd36a
Compare
07bd36a to
ee20c52
Compare
Created a standalone workflow sub module for Cloud Workflows, as the root required workflow_trigger, which essentially created either pub/sub or cloud scheduler job resources as well. The simple workflows aims to create standalone workflow as per now.
PR includes