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

Introduce sem validate FILE to check pipeline files #223

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

caius
Copy link

@caius caius commented Oct 17, 2023

I went looking in the cli for this feature and reached out to support, who pointed me at the /v1alpha/yaml API docs1 so I figured wrapping it in the cli would be useful as I keep trying to reach for it.

It's a painful feedback loop to copy/paste a .semaphore/semaphore.yml from one project to another and edit it, then remember there's some extra stuff from a second project you want to include, so you copy/paste that lot in. Then you push your changes and find a simple syntax error in the pipeline file. Being able to check that before pushing, even though it is limited in what it can check is still super useful and being able to do it from your dev machine is a super tight feedback loop.

Initially I started trying to wedge this under sem get pipeline validate FILE as it's a pipeline related task. That's not where it lives in the API though, so I moved it to a top level command just now. sem validate FILE.

The output isn't great, it seems depending on whether it's parseable YAML or a schema error we can get back different error outputs. Following the lead of the UI, I'm just dumping the response back to the user with a quick note on 👍/👎 first.

Footnotes

  1. Thanks Noelia and Marko!

@caius
Copy link
Author

caius commented Oct 17, 2023

Hey folks! I'm not expecting this to land as-is, I'd like to tidy up the DX of it for starters. (Exit code when YAML is invalid. Perhaps parse the schema violations and present a bit nicely?)

Before I go too much further with this, is there much appetite to having this land upstream in sem? And if so, would it be a top level command? Thanks 😁

@lucaspin
Copy link
Contributor

lucaspin commented Dec 4, 2023

Thanks for submitting this @caius. I think this would be a really helpful addition to the CLI. A few notes that might make this a little bit better:

  1. I think it would be helpful to include a default behavior to sem validate to automatically detect if there's a .semaphore/semaphore.yml and validate it, if no argument is passed. I think even having a sem validate --all to validate all the files in the .semaphore directory would be nice.
  2. Your idea of different exit codes depending on the validation output makes total sense. Just using a non-zero exit code if the validation fails should be helpful enough.
  3. If the validation succeeds, I don't think the command needs to output anything; just the zero exit code should be enough. I think that's how most linters work: they just show you something if there's something wrong.
  4. It would be nice to display the errors in a "pretty" way, but I think we may need to change something on our end to make that easier, because the API itself is not returning the errors properly. This is what I get on an invalid YAML:
$ ./sem validate ./.semaphore/semaphore.yml
Not valid!
"{:malformed, [{\"Type mismatch. Expected String but got Integer.\", \"#/agent/machine/type\"}, {\"Schema does not allow additional properties.\", \"#/blocks/0/task/secretss\"}]}"

@DamjanBecirovic @radwo we should fix the API to expose errors in a more consumable format, like this:

{
  "errors": [
    {
      "key": "#/agent/machine/type",
      "message": "Type mismatch. Expected String but got Integer."
    },
    {
      "key": "#/blocks/0/task/secretss",
      "message": "Schema does not allow additional properties."
    }
  ]
}

And then, displaying them in table should be easy enough, like this:

$ ./sem validate -a
FILE                     KEY                       REASON
.semaphore/semaphore.yml #/blocks/0/task/prologues Schema does not allow additional properties.
.semaphore/semaphore.yml #/blocks/0/task/secretss  Schema does not allow additional properties.

@caius caius force-pushed the cd/yaml-validate branch from baddb67 to 2737272 Compare April 21, 2024 10:47
When you're authoring files locally it's a pain pushing changes to
double-check the YAML syntax is corrected, that you've nested the blocks
correctly and that you've put your `run:` clause at the right level.
Make it easy to validate the syntax of the file from local machines.
@caius caius force-pushed the cd/yaml-validate branch from 2737272 to 0162689 Compare July 31, 2024 13:37
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