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

Improved yaml config and yamllint behaviour #958

Merged
merged 8 commits into from
Jul 9, 2024

Conversation

rhfogh
Copy link
Collaborator

@rhfogh rhfogh commented Jul 5, 2024

Added # %YUAML 1.2 to start fo yaml files;
Changed Yaml Booleans to lower case;
Removed yamllint disable comments

@@ -1,8 +1,9 @@
%YAML 1.2
Copy link
Member

@marcus-oscarsson marcus-oscarsson Jul 8, 2024

Choose a reason for hiding this comment

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

Pay attention to this, it just broke the CI pipeline in mxcube web there is # missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again, there is not '#' missing. If you add in that '#', the command does not work - as far as I could determine in testing. OK, we have still not figured out how to handle this. Still, if you can test and show us that '# %YAML 1.2' works as intended I will say 'thank you' and apologise. Meanwhile, let us not oversimplify this.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry just read your comment above and thought it was simply missing in those files. Why do we need to add % YAML 1.2, raumel uses YAML 1.2 by default as far as I understood, is it for yamllint ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marcus-oscarsson Yes, it is for yamllint. But so far lines with
On:
give a yamllint error, and adding an exception for 'truthy' values to the yaml file seemed to me to be really confusing. It took me long enough to figure out what was going on, and it is not good if the people who write these files get confused about what things mean. If it was only a few key files I guess we could just leave the '%YAML 1.2' out in those files, but I am afraid that you will get errors for any file where the '#' is missing. Alternatively maybe we could disable that rule altogether in the configuration, but that sounds dodgy in itself, and I would also like to keep it to enforce that the capitalisation of Booleans is always lower case, again to avoid confusion. We need to figure out how to do this cleanly, but it is really not obvious how.

As an additional explanation, so far I always, 100% of the time, get a test failure for my pushes. I still do not know why (and I am not at all on top of the .github workflows), so when I saw that there were more test failures than usual, I was not sure whether it was just more of the same, or actually showed a new problem.

Copy link
Member

Choose a reason for hiding this comment

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

I see, Its really strange that some actions fail for you we should really look into that.

Can we force yamllint to use YAML 1.2 without the %YAML 1.2 directive ? Another alternative would be to ignore the .github directory and lint only our configuration files, we know that those are parsed by raumel and support the %YAML directive.

Copy link
Contributor

@fabcor-maxiv fabcor-maxiv Jul 9, 2024

Choose a reason for hiding this comment

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

@rhfogh @marcus-oscarsson

I investigated here: #960

But this seems to be the more active place, so I will comment here now:

The parser that GitHub uses for the Actions workflows is not a full YAML parser. It supports only a subset of YAML. Which subset is supported is unknown to me (and anyone else who is not GitHub apparently). I have not found any clear statement from GitHub so far (this comment from an alleged GitHub staff member at the time for example confirms that they do not support YAML anchors). But seems quite clear that the %YAML 1.2 directive is not supported and breaks their parser.

My recommendation is to leave out the %YAML 1.2 directive out of the YAML files in the .github/workflows directory. For the on issue, we can either go back to on: # yamllint disable-line rule:truthy or write "on": (with the quotes). That should be all that is needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I'll try that. If it works we are fine - yaml inside .github/workflows can safely be left to the few specialists after all. If it does not work we have a problem, I think - it would be really unfortunate and confidence-sapping if we have to have those weird workarounds in the main files. But we shall see.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This passes the tests now. But after all the complications, someone had better sign off on it before I merge it.

FYI I chose using "on": instead of leaving the confusing and obscure 'truthy' exception in.

Copy link
Contributor

@fabcor-maxiv fabcor-maxiv left a comment

Choose a reason for hiding this comment

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

Looks good to me : )

@rhfogh rhfogh merged commit 13cb057 into mxcube:develop Jul 9, 2024
8 checks passed
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.

3 participants