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: [TKC-2966] Store TestWorkflowExecutionRequest config in TestWorkflowExecution #6070

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

povilasv
Copy link
Contributor

@povilasv povilasv commented Dec 5, 2024

Pull request description

Store request config inside TestWorkflowExecution

Checklist (choose whats happened)

  • breaking change! (describe)
  • tested locally
  • tested on cluster
  • added new dependencies
  • updated the docs
  • added a test

Breaking changes

Changes

Fixes

@povilasv povilasv requested a review from a team as a code owner December 5, 2024 10:33
@povilasv povilasv force-pushed the store-config branch 3 times, most recently from 0bdf22e to e6e5757 Compare December 6, 2024 08:56
@povilasv povilasv requested a review from rangoo94 December 6, 2024 09:19
pkg/testworkflows/testworkflowexecutor/executor.go Outdated Show resolved Hide resolved
Comment on lines +476 to +482
for k, _ := range request.Config {
if s, ok := schema[k]; ok && s.Sensitive {
storeConfig = false
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure, but could be that iterating over workflow.Spec.Config could be more correct - as a sensitive property set to default value may also be kind of sensitive (similar to uncertainty when working with PII rules - single information may not be sensitive, but in conjunction with others - it can become sensitive)

But, I guess, we can ignore that for now 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding given users can mark their properties as sensitive, I think we can keep it as is?

I.e. if it's kind of sensitive in relation to others, you can still set it to sensitive, right? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I.e. if it's kind of sensitive in relation to others, you can still set it to sensitive, right?

Yes, but I'm more saying about the default value - i.e. if you specify:

spec:
  config:
    apiUrl: { type: string, default: "my-value", sensitve: true }

And you request execution with config:

{}

The code above will save the configuration properly, despite it actually is using sensitive my-value value.

As mentioned though, I think that we can ignore it for now :)

pkg/testworkflows/utils.go Show resolved Hide resolved
Comment on lines +476 to +482
for k, _ := range request.Config {
if s, ok := schema[k]; ok && s.Sensitive {
storeConfig = false
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I.e. if it's kind of sensitive in relation to others, you can still set it to sensitive, right?

Yes, but I'm more saying about the default value - i.e. if you specify:

spec:
  config:
    apiUrl: { type: string, default: "my-value", sensitve: true }

And you request execution with config:

{}

The code above will save the configuration properly, despite it actually is using sensitive my-value value.

As mentioned though, I think that we can ignore it for now :)

@povilasv povilasv merged commit b60b697 into main Dec 6, 2024
7 checks passed
@povilasv povilasv deleted the store-config branch December 6, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants