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

fix(STONEINTG-583): set scenario status to valid if undefined #389

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

dirgim
Copy link
Collaborator

@dirgim dirgim commented Oct 27, 2023

  • Change the condition for seting scenario status to valid. The previous condition would return false if the condition was not present, so it would not work for scenarios that were created for e2e or load testing
  • See IsStatusConditionFalse for more context - it returns false if status condition is not present

Maintainers will complete the following section

* Change the condition for seting scenario status to valid
  The previous condition would return false if the
  condition was not present, so it would not work
  for scenarios that were created for e2e testing

Signed-off-by: dirgim <[email protected]>
@naftalysh
Copy link

naftalysh commented Oct 29, 2023

Hi @dirgim

  1. What made the integration test scenario status:condition[] array empty and how your PR changes that?

  2. Did and how did it affect the framework.AsKubeDeveloper.IntegrationController.CreateIntegrationTestScenario_beta1 function success?

  3. Does it affects the snapshot creation?
    On what of the below snapshot error types it affects (or on others)?
    Snapshot type and indicators of errors
    https://redhat-internal.slack.com/archives/D059BDTQKC5/p1698325647612289
    https://redhat-internal.slack.com/archives/D059BDTQKC5/p1698325754793929
    https://redhat-internal.slack.com/archives/D059BDTQKC5/p1698326275772429

  4. Since the package is https://github.com/redhat-appstudio/integration-service and relates to the scenario controller , don't we need an update to infra-deployment and thereafter redeployment to a cluster to make the change effective?

  5. Did you create a Bug for that issue?

@dirgim
Copy link
Collaborator Author

dirgim commented Oct 30, 2023

Hi @dirgim

1. What made the integration test scenario status:condition[] array empty and how your PR changes that?

2. Did and how did it affect the framework.AsKubeDeveloper.IntegrationController.CreateIntegrationTestScenario_beta1 function success?

3. Does it affects the snapshot creation?
   On what of the below snapshot error types it affects (or on others)?
   Snapshot type and indicators of errors
   https://redhat-internal.slack.com/archives/D059BDTQKC5/p1698325647612289
   https://redhat-internal.slack.com/archives/D059BDTQKC5/p1698325754793929
   https://redhat-internal.slack.com/archives/D059BDTQKC5/p1698326275772429

4. Since the package is https://github.com/redhat-appstudio/integration-service and relates to the scenario controller , don't we need an update to infra-deployment and thereafter redeployment to a cluster to make the change effective?

5. Did you create a Bug for that issue?
  1. The previous version of the if condition here would return false if the IntegrationTestScenarioValid condition was not present, so it would not trigger for scenarios that were created for e2e or load testing as their .status.conditions field would not be present upon creation. This change makes this correctly detect that it needs to set the IntegrationTestScenarioValid condition to true.
  2. Yes it did, because of the above changes the scenario controller will be able to update the status.conditions with the IntegrationTestScenarioValid condition after they are created via CreateIntegrationTestScenario_beta1.
  3. This does not affect Snapshot creation or handling, only IntegrationTestScenario CR handling in the scenario controller.
  4. Yes, this change will need to be merged and deployed via infra-deployments.
  5. This PR is a follow-up change for STONEINTG-583 to account for this edge case.

Copy link

@jhutar jhutar left a comment

Choose a reason for hiding this comment

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

/lgtm

@naftalysh
Copy link

Hi @dirgim
https://issues.redhat.com/browse/STONEINTG-583 is currently in status Closed
Maybe you should reopen it

@naftalysh
Copy link

Hi @dirgim
When do we expect this change to be merged and deployed via infra-deployments?

@dirgim
Copy link
Collaborator Author

dirgim commented Oct 30, 2023

Hi @naftalysh please be patient as this change is under review by the team and will be merged and deployed after it gets approved. The STONEINTG-583 story cannot be reopened in the current workflow.

Copy link
Member

@dheerajodha dheerajodha left a comment

Choose a reason for hiding this comment

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

LGTM

@naftalysh
Copy link

Thanks @dirgim
I understand

@dirgim
Copy link
Collaborator Author

dirgim commented Oct 31, 2023

The changes have been deployed via redhat-appstudio/infra-deployments#2651

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.

6 participants