Skip to content

Fix: have a good default for approval flow on workflows #1029

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

TomerHeber
Copy link
Collaborator

Issue & Steps to Reproduce / Feature Request

fixes #1023

Solution

  1. During creating isWorkflow is decided based on the template type instead of the sub_environment_configuration.
  2. When updating schema - ignore "approve_plan_automatically" if not set in schema - this is a cleaner solution.
  3. Applied minor updates to acceptance tests.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the default approval flow logic for workflows by basing the workflow decision on the template type and refactoring the environment creation logic. In addition, it updates test expectations and acceptance tests to better align with these changes.

  • Update test expectations for Template calls and workflow type checks.
  • Refactor environment creation into separate functions for template and templateless environments.
  • Adjust approval field update logic and payload generation based on the workflow type.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
env0/resource_environment_test.go Revised test expectations for Template calls and workflow type constant
env0/resource_environment.go Refactored environment creation and payload functions with updated approval flow logic
Comments suppressed due to low confidence (2)

env0/resource_environment_test.go:2442

  • Verify that increasing the expected Template call from 1 to 2 is intentional and that the test covers all interactions to avoid future brittle behavior.
mock.EXPECT().Template(environment.LatestDeploymentLog.BlueprintId).Times(2).Return(template, nil)

env0/resource_environment.go:1040

  • [nitpick] Confirm that determining the workflow status solely based on the template type covers all expected scenarios, particularly if additional types may be introduced later.
isWorkflow := templateType == client.WORKFLOW

@TomerHeber
Copy link
Collaborator Author

/review

@bot-env0 bot-env0 requested a review from a team March 30, 2025 14:45
@TomerHeber TomerHeber requested a review from Copilot April 2, 2025 13:33
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the default behavior for the approval flow in workflow environments by basing decisions on the template type and providing a cleaner update mechanism. Key changes include:

  • Using template.Type instead of sub_environment_configuration to determine workflow environments.
  • Ignoring "approve_plan_automatically" when not set in the schema.
  • Splitting environment creation logic into separate functions for templated and templateless environments, with accompanying updates to acceptance tests and CI workflows.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

File Description
env0/resource_environment_test.go Updated test expectations to match new workflow logic
env0/resource_environment.go Refactored environment creation & validation logic for workflows
env0/data_environment.go Adjusted data reading to reflect the new approval default
.github/workflows/*.yml Updated CI configuration to use Ubuntu 24.04
Comments suppressed due to low confidence (2)

env0/resource_environment_test.go:2442

  • Please confirm that the increased expectation from Times(1) to Times(2) for the Template call is intentional based on the new workflow logic, and update the test case description if necessary.
mock.EXPECT().Template(environment.LatestDeploymentLog.BlueprintId).Times(2).Return(template, nil)

env0/resource_environment.go:1044

  • [nitpick] Ensure that using the equality check (templateType == client.WORKFLOW) reliably distinguishes workflow environments. If there are alternative representations for workflow environments, consider normalizing the input to avoid misclassification.
isWorkflow := templateType == client.WORKFLOW

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have a good default for approval flow on workflows
1 participant