Drop Ruby as a build dependency #740

ffrank · 2024-02-27T10:12:08Z

We have been using Ruby for two things

a test script to find non-conforming YAML files (which has not worked in a long time and had been disabled)
the make yamlfmt target, which is a very hacky approach to having gofmt but for YAML, which is also an odd proposition, given that whitespace has semantic meaning in YAML

This change adds the Python based yamllint to our build dependencies. It has a much smaller footprint compared to bringing in Ruby, especially for folks who want to build mgmt on their regular work/play machines.

The test script now works and gives actionable error output.

The yamlfmt target essentially does the same thing now, though. No automatic "fixing" of YAML formats anymore, but then we probably also don't expect to write that much YAML any longer.

Linter settings were chose so that few changes are needed in existing YAML. It's another benefit of this approach that we can now curate our linter configuration.

ffrank · 2024-02-27T12:45:41Z

I have no idea why the file-mode.sh test suddenly fails in github CI. Works On My Machine.

frebib · 2024-02-27T19:43:37Z

Makefile

@@ -215,7 +215,7 @@ gofmt:
 	find . -maxdepth 9 -type f -name '*.go' -not -path './old/*' -not -path './tmp/*' -not -path './vendor/*' -exec goimports -w {} \;

 yamlfmt:
-	find . -maxdepth 3 -type f -name '*.yaml' -not -path './old/*' -not -path './tmp/*' -not -path './omv.yaml' -exec ruby -e "require 'yaml'; x=YAML.load_file('{}').to_yaml.each_line.map(&:rstrip).join(10.chr)+10.chr; File.open('{}', 'w').write x" \;
+	find . -maxdepth 3 -type f -name '*.yaml' -not -path './old/*' -not -path './tmp/*' -not -path './omv.yaml' | xargs yamllint


Apparently there's a tool called yamlfix that might satisfy this.
https://github.com/lyz-code/yamlfix

frebib · 2024-02-27T19:44:20Z

.github/FUNDING.yml

@@ -1,3 +1,4 @@
+---


We could disable the requirement for this in yamllint if we wanted

purpleidea · 2024-02-27T19:50:51Z

I have no idea why the file-mode.sh test suddenly fails in github CI. Works On My Machine.

Race bugs in testing env. I will disable it.

purpleidea · 2024-02-27T19:51:45Z

then we probably also don't expect to write that much YAML any longer.

Quite true. I expect a few incoming config file things, but in general it's not as much of an issue.

ffrank · 2024-02-29T20:24:06Z

In github CI I realized we have two more ruby users: mdl and fpm. fpm is not so painful because it doesn't need to be part of the build, and if we want to run it in CI, github actions can still just run ruby setup actions.

However, mdl is an entire different story. I tried substituting it with pymarkdownlnt, which has a limitation around tables (I can work around), but also has an annoying bug that occurs when indented blocks appear in enumerations or bullet point lists, and TABs are used for indentation. This begs the questions:

Why do we insist on tabs in markdown? It's not best practice and we explicitly disable the MD010 rule that would warn us about this. Linting life would be much easier without tabs. Would spaces be so bad here?

(This mostly happens in docs/language-guide.md.)

purpleidea · 2024-02-29T20:29:14Z

Why do we insist on tabs

I love my tabs. For many reasons unfortunately.

Accessibility
Not having to hammer my space bar to indent
Easy of use for when you want to see more or less indents

...

purpleidea · 2024-02-29T20:29:52Z

(And in theory we should be adding more markdown docs as time goes on, so we definitely want to keep the validator.)

ffrank · 2024-02-29T20:33:13Z

To illustrate the issue in the previous comment:

$ pymarkdownlnt scan docs/language-guide.md 

Unexpected Error(BadTokenizationError): An unhandled error occurred processing the document.

The issue is a failed assertion in this parser. I believe that it's caused by a bug, and I have been looking into it, but debugging someone else's parser/lexer code is...challenging.

purpleidea · 2024-02-29T20:51:03Z

While I love the idea of dropping the use of any ruby, it being there is not personally blocking me atm.

But if you're really keen on this issue, maybe this would help though: https://github.com/gomarkdown/markdown ?

ffrank · 2024-02-29T22:24:15Z

Oh I definitely want to be sure we keep linting the markdown. We should even work towards disabling fewer rules imo.

So yeah I filed jackdewinter/pymarkdown#1015 and will also look at the gomarkdown thingy.

ffrank · 2024-03-02T07:41:50Z

The tip of this now has a shape like I could imagine it. Let me know your thoughts please.

If you all find this an acceptable way forward, I will squash it down.

Note that I went for pipenv over just jamming pip packages into everyone's systems (except pipenv itself...apparently it's best practice to install it through pip). I hope this will be more sustainable (and not more fragile).

purpleidea · 2024-03-03T00:15:20Z

Wow, lots of neat work, thanks Felix!

I've just had a short read through as a diff of the whole thing, rather than each commit separately. There are clearly a bunch of great changes I can see already.

There are a few unknowns I'd need to dig into first. My first question is that if we're moving from ruby to python, is this a major improvement? It definitely seems to be from a packaging point of view. I'd like to rule out gomarkdown though first, at least for the mdl alternative.

Lastly, how did you want to squash? (I do not recommend squashing all of it into a single commit.) If there are a few stand-alone / easy commits to squash and/or merge early, maybe I can make this easier to review by merging those first?

I have a very large feature branch that I'll be merging shortly, and my goal is to merge this right after that. I don't think there are would be any major conflicts, but will do it in that order just to be safe.

Cheers!

ffrank · 2024-03-05T17:17:01Z

There are a few unknowns I'd need to dig into first. My first question is that if we're moving from ruby to python, is this a major improvement? It definitely seems to be from a packaging point of view. I'd like to rule out gomarkdown though first, at least for the mdl alternative.

Python is already a dependency, and no modern Linux box can function without python. It's a lot less noise on contributor's machines in this way. Ruby is rather obscure. Even Puppet users don't install it anymore, since Puppet vendors its own Ruby (go figure).

I will try replacing pymarkdown with gomarkdown before I proceed.

Lastly, how did you want to squash? (I do not recommend squashing all of it into a single commit.) If there are a few stand-alone / easy commits to squash and/or merge early, maybe I can make this easier to review by merging those first?

Mainly I want to get rid of some intermediary steps like ruby-yaml-fix -> yamllint -> yamlfix. Also if I go for gomarkdown, any mention of pymarkdown needs not be in our history.

But yes, otherwise I'm no fan of single-commit branches either. Some history can be helpful in archaeology down the line.

ffrank · 2024-03-05T18:39:17Z

So gomarkdown does not come with linting functionality. There is gomarkdown/mdfmt, but it's unmaintained and go build fails.

A slightly better maintained version is at https://github.com/shurcooL/markdownfmt/ and it does build, but it's not really customizable, and at a glance, it seems to do the opposite of what we want, e.g. joining our nice and carefully crafted 80 character line paragraphs into single long lines.

purpleidea · 2024-03-05T18:41:51Z

haha, okay! I'm okay with the python route for now if you think that's preferable. I'm a bit concerned about the open issue and wouldn't mind waiting until that's fixed before we swap it though.

Would you be able to rebase/squash your commits into a few separate ones based on logical functionality that you think makes sense and I'll review and merge those?

purpleidea · 2024-03-05T18:42:21Z

Or I could pick off the easy ones first if you'd like. LMK

ffrank · 2024-03-05T19:03:49Z

I will clean up and ping you.

ffrank · 2024-03-06T15:05:40Z

Okay, go.

ffrank · 2024-03-10T18:10:11Z

@purpleidea this is ready for review.

ffrank · 2024-04-01T20:08:09Z

Update, I'm in communication with @jackdewinter in jackdewinter/pymarkdown#1015 in order to solve the limitation around tab characters. Once the fix works, it will allow us to remove the workaround from test-markdownlint.sh.

But I feel that we can merge as is now, and remove the workaround later, so this needs not be blocked.

purpleidea · 2024-04-03T01:11:55Z

test/test-commit-message.sh

@@ -10,11 +10,12 @@ echo running "$0"
 ROOT=$(dirname "${BASH_SOURCE}")/..
 cd "${ROOT}" || exit 1

-commit_title_regex='^\([a-z0-9]\(\(, \)\|[a-z0-9]\)\+[a-z0-9]: \)\+[A-Z0-9][^:]\+[^:.]$'
+commit_title_regex='^\([a-z0-9]\(\(, \)\|[a-z0-9]\)*[a-z0-9]: \)\+[A-Z0-9][^:]\+[^:.]$'


What does this do?

It drops the random requirement that the "topic:" prefix has at least three characters in the topic.

purpleidea · 2024-04-03T01:13:11Z

test/test-commit-message.sh

@@ -183,5 +184,13 @@ then
 			test_commit_message_common_bugs $commit
 		done
 	fi
+else # assume local branch


purpleidea · 2024-04-03T01:20:18Z

test/test-commit-message.sh

@@ -155,42 +157,30 @@ test_commit_message_body() {
 if [[ -n "$TRAVIS_PULL_REQUEST_SHA" ]]
 then
 	commits=$(git log --format=%H origin/${TRAVIS_BRANCH}..${TRAVIS_PULL_REQUEST_SHA})
-	[[ -n "$commits" ]]


Why are we nuking everything from here to the end?

We just avoid copy/paste by doing the for commit in $commits work at the bottom of the script.

purpleidea · 2024-04-03T01:20:28Z

test/test-commit-message.sh

 	then
 		echo "FAIL: Commit message should match the following regex: '$commit_title_regex'"
+		echo "('$title' does not match)"


purpleidea · 2024-04-03T01:33:11Z

I've cherry-picked a few of these commits. I think we should hold off on the markdown/yaml thing for the moment. I need to have a deeper look into it all. Thanks for the fun commits!

We pull in pipenv and install pip packages for yamlfix and pymarkdownlnt into the virtualenv. This allows us to drop mdl and later replace the ruby hack in Makefile that currently does our yamlfmt.

At the same time, introduce yamlfix for a proper "make yamlfmt" workflow that does not rely on the old ruby based hack. Also change existing YAML to reflect the result.

Also now shows the title line of offensive commit message.

Debian also now forbids installing pips system-wide, so pip3 install will just fail without messy force flags, so going the pipx route is more compatible. It is brittle in new ways, but probably less so than messing with system pip packages.

ffrank force-pushed the noruby branch from 5dd0522 to 468cceb Compare February 27, 2024 12:13

ffrank force-pushed the noruby branch from eb1b3f9 to f4bfca5 Compare February 27, 2024 12:49

frebib approved these changes Feb 27, 2024

View reviewed changes

ffrank force-pushed the noruby branch 2 times, most recently from 9034c4c to 8570028 Compare March 1, 2024 23:44

ffrank marked this pull request as draft March 5, 2024 17:17

ffrank force-pushed the noruby branch from 8570028 to a885fa0 Compare March 6, 2024 14:18

ffrank marked this pull request as ready for review March 6, 2024 15:05

ffrank mentioned this pull request Apr 1, 2024

Improve commit message test #751

Open

purpleidea reviewed Apr 3, 2024

View reviewed changes

ffrank force-pushed the noruby branch from a885fa0 to 837014a Compare May 10, 2024 21:40

ffrank and others added 4 commits September 8, 2024 17:10

test: markdownlint: Replace mdl with pymarkdownlnt

e7b6edb

We pull in pipenv and install pip packages for yamlfix and pymarkdownlnt into the virtualenv. This allows us to drop mdl and later replace the ruby hack in Makefile that currently does our yamlfmt.

test: Use yamllint over ruby for testing YAML formatting

b7cfc88

At the same time, introduce yamlfix for a proper "make yamlfmt" workflow that does not rely on the old ruby based hack. Also change existing YAML to reflect the result.

test: commitmessage: Simplify, actually run without PR

ecc66b9

Also now shows the title line of offensive commit message.

test: mardownlint: WiP removal of the workaround of leading tasbs issues

fcd4cd0

ffrank force-pushed the noruby branch from 837014a to 6487b3c Compare September 8, 2024 15:51

Felix Frank added 3 commits September 8, 2024 17:59

test: Fix syntax in pymarkdownlnt rule

4786c6c

provisioner: Make yamllint happy

2beedc1

ffrank force-pushed the noruby branch from 6487b3c to 2beedc1 Compare September 8, 2024 18:59

purpleidea force-pushed the master branch from 2431df9 to 71a1efd Compare October 30, 2024 04:41

purpleidea force-pushed the master branch 3 times, most recently from b8072b2 to 380004b Compare November 23, 2024 06:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop Ruby as a build dependency #740

Drop Ruby as a build dependency #740

ffrank commented Feb 27, 2024

ffrank commented Feb 27, 2024

frebib Feb 27, 2024

frebib Feb 27, 2024

purpleidea commented Feb 27, 2024

purpleidea commented Feb 27, 2024

ffrank commented Feb 29, 2024

purpleidea commented Feb 29, 2024

purpleidea commented Feb 29, 2024

ffrank commented Feb 29, 2024

purpleidea commented Feb 29, 2024

ffrank commented Feb 29, 2024

ffrank commented Mar 2, 2024 •

edited

Loading

purpleidea commented Mar 3, 2024

ffrank commented Mar 5, 2024

ffrank commented Mar 5, 2024

purpleidea commented Mar 5, 2024

purpleidea commented Mar 5, 2024

ffrank commented Mar 5, 2024

ffrank commented Mar 6, 2024

ffrank commented Mar 10, 2024

ffrank commented Apr 1, 2024 •

edited

Loading

purpleidea Apr 3, 2024

ffrank Apr 3, 2024

purpleidea Apr 3, 2024

purpleidea Apr 3, 2024

ffrank Apr 3, 2024

purpleidea Apr 3, 2024

purpleidea commented Apr 3, 2024

Drop Ruby as a build dependency #740

Are you sure you want to change the base?

Drop Ruby as a build dependency #740

Conversation

ffrank commented Feb 27, 2024

ffrank commented Feb 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purpleidea commented Feb 27, 2024

purpleidea commented Feb 27, 2024

ffrank commented Feb 29, 2024

purpleidea commented Feb 29, 2024

purpleidea commented Feb 29, 2024

ffrank commented Feb 29, 2024

purpleidea commented Feb 29, 2024

ffrank commented Feb 29, 2024

ffrank commented Mar 2, 2024 • edited Loading

purpleidea commented Mar 3, 2024

ffrank commented Mar 5, 2024

ffrank commented Mar 5, 2024

purpleidea commented Mar 5, 2024

purpleidea commented Mar 5, 2024

ffrank commented Mar 5, 2024

ffrank commented Mar 6, 2024

ffrank commented Mar 10, 2024

ffrank commented Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purpleidea commented Apr 3, 2024

ffrank commented Mar 2, 2024 •

edited

Loading

ffrank commented Apr 1, 2024 •

edited

Loading