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

test(sql): Add extra test for pipelineRef feature flag #4760

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

edgarulg
Copy link
Contributor

@edgarulg edgarulg commented Jul 4, 2024

I added more tests in executionLaucherTest regarding PipelineRef feature include as part of #4749

@edgarulg edgarulg requested a review from dbyron-sf July 4, 2024 03:50
@@ -332,6 +337,120 @@ public void testExcludeSpinnakerAccountsFromPipeline() throws Exception {
assertThat(pipelineExecution.getAuthentication().getAllowedAccounts()).isEqualTo(Set.of());
}

@DisplayName(
"when pipelineRef.enabled: true, the object mapper can deserialize the PipelineTrigger into PipelineRefTrigger")
Copy link
Contributor

@dbyron-sf dbyron-sf Jul 4, 2024

Choose a reason for hiding this comment

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

Isn't the operation here about converting from pipeline config to pipeline execution? It would help me if the description focused on this more than the implementation details.

Maybe this isn't 100% of what's happening since triggers come in from manual execution, and from parent pipeline executing children. But, these triggers never have full on execution contexts, do they? I mean, the pipeline in question hasn't run yet, right? If that's true, wouldn't we expect the same behavior whether pipelineRef.enabled is true or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the ExecutionLauncher the pipeline hasn't run yet. The purpose of the ExecutionLauncher is take a pipeline config and parse it into a execution context then store it using SqlExecutionRepository.

The main idea of my new tests is that even with the customTrigger configured in the objectMapper the ExecutionLauncher can start a new execution. I just include an assert to validate that the customTrigger is being use as well but the same behavior is kept.
I will change the description on the test to avoid implementation details

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