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

feat:Ability to choose default setting for Deployment Triggers for new Environments #164

Conversation

GiliFaroEnv0
Copy link
Contributor

@GiliFaroEnv0 GiliFaroEnv0 commented Nov 22, 2021

…w Environments

Issue & Steps to Reproduce / Feature Request

Add ability to set default CD and PR plan by project policy

Solution

Add ability to set default CD and PR plan by project policy

@GiliFaroEnv0 GiliFaroEnv0 force-pushed the feat-Add-ability-to-set-default-CD-and-PR-plan-by-project-policy branch from a0c5a54 to 029f93c Compare November 22, 2021 14:05
@GiliFaroEnv0
Copy link
Contributor Author

@GiliFaroEnv0 GiliFaroEnv0 force-pushed the feat-Add-ability-to-set-default-CD-and-PR-plan-by-project-policy branch from a091d51 to c070c4c Compare November 23, 2021 12:26
@GiliFaroEnv0 GiliFaroEnv0 changed the title feat:Ability to choose default setting for Deployment Triggers for ne… feat:Ability to choose default setting for Deployment Triggers for new Environments Nov 23, 2021
Copy link
Contributor

@yaronya yaronya left a comment

Choose a reason for hiding this comment

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

Nice work, left a some comments

client/model.go Outdated
SkipApplyWhenPlanIsEmpty bool `json:"skipApplyWhenPlanIsEmpty"`
DisableDestroyEnvironments bool `json:"disableDestroyEnvironments"`
SkipRedundantDeployments bool `json:"skipRedundantDeployments"`
Id string `json:"id"`
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't send the id on the project update payload, the id is a part of the path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

client/model.go Outdated
SkipApplyWhenPlanIsEmpty bool `json:"skipApplyWhenPlanIsEmpty"`
DisableDestroyEnvironments bool `json:"disableDestroyEnvironments"`
SkipRedundantDeployments bool `json:"skipRedundantDeployments"`
UpdatedBy string `json:"updatedBy"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, this is a computed attribute and shouldn't be sent on payload...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type: schema.TypeBool,
Description: "Run Terraform Plan on Pull Requests for new environments targeting their branch default value",
Optional: true,
Default: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since all other fields here don't have a default value setting, I think we should change it to either make all of them with default values, or none.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +24 to +31
- **continuous_deployment_default** (Boolean) Redeploy on every push to the git branch default value
- **disable_destroy_environments** (Boolean) Disallow destroying environment in the project
- **id** (String) id of the policy
- **include_cost_estimation** (Boolean) Enable cost estimation for the project
- **number_of_environments** (Number) Max number of environments a single user can have in this project, 0 indicates no limit
- **number_of_environments_total** (Number) Max number of environments in this project, 0 indicates no limit
- **requires_approval_default** (Boolean) Requires approval default value when creating a new environment in the project
- **run_pull_request_plan_default** (Boolean) Run Terraform Plan on Pull Requests for new environments targeting their branch default value
Copy link
Contributor

Choose a reason for hiding this comment

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

why they not near each other - both of them belong trigger policy.
Plus I think id should be the first one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its generated

Comment on lines +41 to +47
- **continuous_deployment_default** (Boolean) Redeploy on every push to the git branch default value
- **disable_destroy_environments** (Boolean) Disallow destroying environment in the project
- **include_cost_estimation** (Boolean) Enable cost estimation for the project
- **number_of_environments** (Number) Max number of environments a single user can have in this project, 0 indicates no limit
- **number_of_environments_total** (Number) Max number of environments in this project, 0 indicates no limit
- **requires_approval_default** (Boolean) Requires approval default value when creating a new environment in the project
- **run_pull_request_plan_default** (Boolean) Run Terraform Plan on Pull Requests for new environments targeting their branch default value
Copy link
Contributor

Choose a reason for hiding this comment

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

same. write one near to other

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its generated

@HeverFarber HeverFarber added ready to merge PR approved - can be merged once the PR owner is ready and removed pending final review labels Nov 24, 2021
@GiliFaroEnv0 GiliFaroEnv0 merged commit 8536813 into main Nov 25, 2021
@GiliFaroEnv0 GiliFaroEnv0 deleted the feat-Add-ability-to-set-default-CD-and-PR-plan-by-project-policy branch November 25, 2021 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge PR approved - can be merged once the PR owner is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants