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

Enable workflow validation against JSON spec #13125

Closed
wants to merge 5 commits into from

Conversation

HenryNguyen5
Copy link
Collaborator

No description provided.

@HenryNguyen5 HenryNguyen5 requested a review from a team as a code owner May 7, 2024 16:51
@HenryNguyen5 HenryNguyen5 requested review from cedric-cordenier and removed request for cedric-cordenier May 7, 2024 16:51
core/services/workflows/models_test.go Outdated Show resolved Hide resolved
core/services/workflows/models_test.go Outdated Show resolved Hide resolved
core/services/workflows/models_test.go Outdated Show resolved Hide resolved

actions:
- id: "an-action"
- id: "an-action@1"
config: {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IRL workflow scenarios seem to always have a config, hence why this is a required field rather than a nullable one. It does look at little odd when we're using fixture workflows though

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does look at little odd when we're using fixture workflows though

Could this fixture be made more realistic, then?

core/services/workflows/models_test.go Outdated Show resolved Hide resolved
core/services/workflows/models_test.go Outdated Show resolved Hide resolved
core/services/workflows/models_test.go Outdated Show resolved Hide resolved
core/services/workflows/models_test.go Outdated Show resolved Hide resolved
core/services/workflows/models_test.go Outdated Show resolved Hide resolved
core/services/workflows/models_yaml.go Outdated Show resolved Hide resolved
@@ -23,15 +23,15 @@ import (

const hardcodedWorkflow = `
triggers:
- id: "mercury-trigger"
- id: "mercury-trigger@1"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we have some version redundancy/conflict now that the ID contains the version. The capability info struct needs to be refactored such that the version is derived from the ID rather than being a separate entity.

This is tracked in:

Copy link
Contributor

Choose a reason for hiding this comment

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

@HenryNguyen5 I think that means that the hardcoded workflow will be broken if we merge this PR right? Since the spec will require versions, but that won't have entries in the capability registry

Is there anything we can do to ensure that these changes don't break things in the meantime?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're talking about staging, right?
I think the non-breaking change would be to update the staging env such that the id entries in staging contain the version too. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but I'm also talking about the various CapabilityInfo objects kicking around. By modifying this without modifying those, aren't we ensuring that we won't be able to fetch existing capabilities (since new workflows will fail validation without a version?)

I would love for us to make these changes in a way that maintains functionality -- my main concern here is ensuring that what is on develop is always in a working state.

krehermann
krehermann previously approved these changes May 15, 2024
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.

4 participants