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: validate workflow schema #159

Closed
wants to merge 35 commits into from

Conversation

Namyalg
Copy link
Contributor

@Namyalg Namyalg commented Apr 23, 2022

Description

This PR consists of a script and a workflow that validates the yml schema of a workflow file

Related issue(s)
Issue #150

This is sample run on 2 erroneous files

@Namyalg Namyalg changed the title Validate workflow schema feat: validate workflow schema Apr 23, 2022
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

Thanks a lot. I made few suggestions. Once you apply them, please show workflow in action in some test repo

.github/scripts/validate-workflow-schema.js Outdated Show resolved Hide resolved
.github/scripts/validate-workflow-schema.js Outdated Show resolved Hide resolved
.github/scripts/validate-workflow-schema.js Outdated Show resolved Hide resolved
.github/workflows/validate-workflow-schema.yml Outdated Show resolved Hide resolved
.github/workflows/validate-workflow-schema.yml Outdated Show resolved Hide resolved
@Namyalg
Copy link
Contributor Author

Namyalg commented Apr 27, 2022

@derberg , In case 2 or more yml files are created during a PR, and the first one fails, currently the action will stop at the first failed file. Is it a good idea to do this or validate both the files and then report the error ?

@derberg
Copy link
Member

derberg commented Apr 27, 2022

@Namyalg best developer experience would be to validate all the files and throw error at the end of validation with list of all affected files, end related errors

just catch errors into a Map where you keep filename and error and then at the end, if size is bigger than 0 just throw error with all details

@Namyalg
Copy link
Contributor Author

Namyalg commented Apr 27, 2022

Okay, working on this

@Namyalg
Copy link
Contributor Author

Namyalg commented Apr 27, 2022

I have made the changes suggested

This is a sample workflow run on erroneous workflow files

This is sample on a file adhering to the schema

Few points :

  • The JSON schema has been stored in a file in scripts . I was facing some issues due to the async call to axios, hence fs.readFileSync has been used
  • On encountering errors in file(s), the name of the file and the log is shown. The workflow fails (using actions/@core)
  • Without installing actions/@core, a package not found error occurred (As it was earlier mentioned that actions/@core is a part of the actions/github-script action)

@Namyalg Namyalg requested a review from derberg April 29, 2022 03:54
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

looking better, left some comments, please have a look

.github/workflows/validate-workflow-schema.yml Outdated Show resolved Hide resolved
.github/workflows/validate-workflow-schema.yml Outdated Show resolved Hide resolved

- name: Get changed files
id: changed-files
uses: tj-actions/[email protected]
Copy link
Member

Choose a reason for hiding this comment

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

can we make sure it check only changes in files in .github/workflows ?

.github/workflows/validate-workflow-schema.yml Outdated Show resolved Hide resolved
.github/workflows/validate-workflow-schema.yml Outdated Show resolved Hide resolved
@derberg
Copy link
Member

derberg commented Oct 19, 2022

@Namyalg hey, need a hand?

@Namyalg
Copy link
Contributor Author

Namyalg commented Oct 22, 2022

Hey @derberg, working on the changes

@derberg
Copy link
Member

derberg commented Nov 14, 2022

@Namyalg fyi there are still 2 not addressed comments

@Namyalg
Copy link
Contributor Author

Namyalg commented Jan 6, 2023

Hey @derberg, apologize for the delay.
The script to validate schemas reads from the URL, based on the reference script written by @KhudaDad414

@derberg
Copy link
Member

derberg commented Jan 12, 2023

@Namyalg no worries 👍🏼

@KhudaDad414 can you please have another look?

KhudaDad414
KhudaDad414 previously approved these changes Jan 13, 2023
Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

Let's see this in action :) thanks @Namyalg 👏

@Namyalg
Copy link
Contributor Author

Namyalg commented Jan 13, 2023

Shall I delete the test file one and two created for verification ?

@KhudaDad414
Copy link
Member

@Namyalg sure, since we do not need them.

@Namyalg
Copy link
Contributor Author

Namyalg commented Jan 13, 2023

Done with the deletions 👍

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

We are almost ready to go, but since the creation of this PR, some changes were introduces in project in regards to "global" files.

Lemme know if you need more explanation

@Namyalg
Copy link
Contributor Author

Namyalg commented Mar 14, 2023

Okay @derberg , will look into it

@derberg
Copy link
Member

derberg commented Mar 27, 2023

@Namyalg need help?

@derberg
Copy link
Member

derberg commented Apr 5, 2023

@Namyalg I hope you plan to complete it as you spent a lot of time on it.

We have one contributor that is willing to complete it for you, but I want to make sure you are ok with it

@Namyalg
Copy link
Contributor Author

Namyalg commented Apr 5, 2023

Hello @derberg , working on it.
I would like to collaborate with the other contributor.

Copy link
Member

derberg commented Apr 6, 2023

@14Richa I know you were interested to help with this one
@Namyalg thanks for quick response 🙏

@14Richa
Copy link
Contributor

14Richa commented Apr 8, 2023

Hey @Namyalg, let me know if you need any help. You can ping me on slack as well. :)

Always happy to help!!

@14Richa
Copy link
Contributor

14Richa commented Apr 8, 2023

Thanks, @derberg. I will get in touch with @Namyalg and make the pr ready to merge asap together. :)

@derberg
Copy link
Member

derberg commented Apr 19, 2023

any success? maybe we should just start from scratch?

we need this validation as we have lots of workflows, and I caused some errors recently just because of syntax error in a workflow 🙃 😄

@Namyalg
Copy link
Contributor Author

Namyalg commented Apr 23, 2023

Yes, I think we can start from scratch @derberg
Where should I get started ?

Copy link
Member

derberg commented Apr 24, 2023

@Namyalg if you want to continue with this one, no need to start from scratch. There are minor stuff left. I suggested from scratch in case you want to let it go.

did you have a change to sync with @14Richa and get some help? Both of you are in our Slack

@KhudaDad414
Copy link
Member

closing since it has been done here: #238

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