ci(i): Add YAML linter #2241

shahzadlone · 2024-01-22T10:26:25Z

Relevant issue(s)

Resolves Add a YAML linter #2245

Description

Basic YAML linter Action
Fix all YAML linter errors
PR title change when using the manual action to combine dependabot PRs
Update default options of the combine action according to our common usage

How has this been tested?

CI checks for this now

Specify the platform(s) on which this was tested:

WSL (Manjaro)

shahzadlone · 2024-01-22T10:27:05Z

.github/workflows/combine-bot-prs.yml

@@ -66,7 +66,11 @@ jobs:
              if (branch.startsWith('${{ github.event.inputs.branchPrefix }}')) {
                console.log('Branch matched prefix: ' + branch);
                let statusOK = true;
-                if(${{ github.event.inputs.mustBeGreen }}) {
+                if ($ {


Most of this code was auto formatted through prettier.

shahzadlone · 2024-01-22T10:33:30Z

The [Optional] action that fails is because of the check that enforces to use branch-flow if ever touched files in:

    paths:
      - '.github/workflows/preview-ami-with-terraform-plan.yml'
      - '.github/workflows/build-then-deploy-ami.yml'
      - 'tools/cloud/aws/**'

Should be fine to dismiss as the only changes in that file are trimming ending white spaces.

AndrewSisley

Looks mostly positive, however I think it has damaged readability in at least one location and needs a bit of tweaking IMO.

AndrewSisley · 2024-01-22T13:54:09Z

.github/workflows/combine-bot-prs.yml

-                  const [{ commit }] = result.repository.pullRequest.commits.nodes;
+                  const [{
+                    commit
+                  }] = result.repository.pullRequest.commits.nodes;


thought: This looked nicer before

This line should pass either way now. Can revert this one to look like before.

AndrewSisley · 2024-01-22T13:54:33Z

.github/workflows/combine-bot-prs.yml

+                branch,
+                prString
+              }
+              of branchesAndPRStrings) {


todo: This looked much nicer before. What was responsible for this change? Can we please tweak whatever did this so that it does not?

This is the fault of the prettier formatter i was using for Javascript code. Should pass still with how it looked before, will fix.

AndrewSisley · 2024-01-22T14:23:34Z

Makefile

+else
+	$(info YAML linter 'yamllint' and `pip` both not found.)
+endif
+ifeq (, $(shell which yamllint)) # If yamllint still not installed then try this: 


question: Sorry I missed this on first review. Are you sure this is really worth it? I am somewhat sceptical - did you hit problems in the CI?

The first method failed to install on one of the machines (not ci, locally) so provided the alternative. This just ensure like previously when we did make deps:lint it can install the linter regardless of which machine we are on. Not as easy to do this since its not a go tool haha like the go linter which would install easily.

This only needs to work in the CI though really no? I'd rather not have us maintain a list of every possible installation pathway for every non-go dependency we end up having. Not to mention that it will 'break' every time a new pathway becomes available/common.

Can just add a line in the readme noting this linter is a dependency, and then the machine owner can install it in whatever way they prefer. Having that choice if probably preferred by a bunch of people who may have multiple package managers installed (e.g. if they have one main, and then at some point in the distant past had to install another for a very specific nuisance package, and that one-off gets auto-picked up by us first).

I'm also not sure people would appreciate random sudo calls in our make file either.

Yea, I guess that's one way so we don't have to worry about the nastiness of building and maintaining every possible way to download a dependency. We could perhaps adopt the strategy that if it is not-a-go tool we just document it and not provide utility methods to install it.

will remove, unless someone else has a stronger opinion

I'm also in favour or removing it 👍

Removed, please re-review

.goreleaser.yaml

codecov · 2024-01-22T22:17:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (958e471) 74.03% compared to head (eab81f7) 74.07%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2241      +/-   ##
===========================================
+ Coverage    74.03%   74.07%   +0.03%     
===========================================
  Files          256      256              
  Lines        25739    25739              
===========================================
+ Hits         19055    19064       +9     
+ Misses        5364     5357       -7     
+ Partials      1320     1318       -2

Flag	Coverage Δ
all-tests	`74.07% <ø> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 7 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 958e471...eab81f7. Read the comment docs.

AndrewSisley

LGTM :) Thanks Shahzad

fredcarle

👍

## Relevant issue(s) - Resolves #2245 ## Description - Basic YAML linter Action - Fix all YAML linter errors - PR title change when using the manual action to combine dependabot PRs - Update default options of the combine action according to our common usage ## How has this been tested? - CI checks for this now Specify the platform(s) on which this was tested: - WSL (Manjaro)

## Relevant issue(s) - Resolves sourcenetwork#2245 ## Description - Basic YAML linter Action - Fix all YAML linter errors - PR title change when using the manual action to combine dependabot PRs - Update default options of the combine action according to our common usage ## How has this been tested? - CI checks for this now Specify the platform(s) on which this was tested: - WSL (Manjaro)

shahzadlone added ci/build This is issue is about the build or CI system, and the administration of it. code quality Related to improving code quality labels Jan 22, 2024

shahzadlone added this to the DefraDB v0.10 milestone Jan 22, 2024

shahzadlone self-assigned this Jan 22, 2024

shahzadlone commented Jan 22, 2024

View reviewed changes

shahzadlone requested a review from a team January 22, 2024 12:58

AndrewSisley reviewed Jan 22, 2024

View reviewed changes

fredcarle reviewed Jan 22, 2024

View reviewed changes

.goreleaser.yaml Show resolved Hide resolved

shahzadlone added 4 commits January 22, 2024 16:19

PR: Add linting for YAML files

ae12eff

PR: make rule for installing and running yamllint

f6e0e52

PR: Resolve all YAML lint errors

0fd3e36

PR: Use the configuration file

3dac06b

shahzadlone force-pushed the lone/yaml-lint branch from 88cc751 to 38cf79a Compare January 22, 2024 21:38

shahzadlone requested review from fredcarle and AndrewSisley January 22, 2024 21:41

shahzadlone force-pushed the lone/yaml-lint branch from 38cf79a to 66ebea4 Compare January 22, 2024 21:59

shahzadlone added 2 commits January 22, 2024 17:23

PR: Update combining action default options

1813ee0

PR: Do the PR feedbacks

16ce856

shahzadlone force-pushed the lone/yaml-lint branch from 66ebea4 to 16ce856 Compare January 22, 2024 22:23

AndrewSisley approved these changes Jan 23, 2024

View reviewed changes

fredcarle approved these changes Jan 23, 2024

View reviewed changes

Merge branch 'develop' into lone/yaml-lint

eab81f7

shahzadlone merged commit ed2285c into sourcenetwork:develop Jan 23, 2024
32 of 33 checks passed

shahzadlone deleted the lone/yaml-lint branch January 23, 2024 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci(i): Add YAML linter #2241

ci(i): Add YAML linter #2241

shahzadlone commented Jan 22, 2024 •

edited

Loading

shahzadlone Jan 22, 2024

shahzadlone commented Jan 22, 2024

AndrewSisley left a comment

AndrewSisley Jan 22, 2024

shahzadlone Jan 22, 2024

shahzadlone Jan 22, 2024

AndrewSisley Jan 22, 2024

shahzadlone Jan 22, 2024

shahzadlone Jan 22, 2024

AndrewSisley Jan 22, 2024

shahzadlone Jan 22, 2024

AndrewSisley Jan 22, 2024

AndrewSisley Jan 22, 2024

shahzadlone Jan 22, 2024

shahzadlone Jan 22, 2024

fredcarle Jan 22, 2024

shahzadlone Jan 22, 2024

codecov bot commented Jan 22, 2024 •

edited

Loading

AndrewSisley left a comment

fredcarle left a comment

ci(i): Add YAML linter #2241

ci(i): Add YAML linter #2241

Conversation

shahzadlone commented Jan 22, 2024 • edited Loading

Relevant issue(s)

Description

How has this been tested?

Choose a reason for hiding this comment

shahzadlone commented Jan 22, 2024

AndrewSisley left a 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

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

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

codecov bot commented Jan 22, 2024 • edited Loading

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

shahzadlone commented Jan 22, 2024 •

edited

Loading

codecov bot commented Jan 22, 2024 •

edited

Loading