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

BRE-370 - Add rule settings for document-start and document-end markers #333

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

vgrassia
Copy link
Member

🎟️ Tracking

📔 Objective

This PR adds rule settings for document-start and document-end markers for yamllint. If a document-start or document-end marker is in the workflow file, the workflow linter will fail with an error.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@vgrassia vgrassia requested a review from a team as a code owner October 15, 2024 15:09
@vgrassia vgrassia enabled auto-merge (squash) October 15, 2024 15:09
@Eeebru
Copy link
Contributor

Eeebru commented Oct 15, 2024

So we must remove all document-start from all workflow files then? Should this be on hold until we clear workflow files up?

Copy link

Logo
Checkmarx One – Scan Summary & Details707072d5-db73-4458-bf07-cf2f4eff98a6

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Unpinned Actions Full Length Commit SHA /workflow-linter.yml: 51 Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...

Fixed Issues

Severity Issue Source File / Package
MEDIUM Absolute_Path_Traversal /download-artifacts/node_modules/unzip-stream/test.js: 20
LOW Log_Forging /download-artifacts/node_modules/minimist/example/parse.js: 3
LOW Log_Forging /download-artifacts/node_modules/unzip-stream/test.js: 6
LOW Log_Forging /download-artifacts/node_modules/uuid/dist/uuid-bin.js: 26
LOW Log_Forging /download-artifacts/node_modules/uuid/dist/uuid-bin.js: 26
LOW Log_Forging /get-keyvault-secrets/node_modules/uuid/dist/uuid-bin.js: 26
LOW Log_Forging /get-keyvault-secrets/node_modules/uuid/dist/uuid-bin.js: 26

@mimartin12
Copy link
Contributor

So we must remove all document-start from all workflow files then? Should this be on hold until we clear workflow files up?

Good call out.
Linking some docs here:
https://yamllint.readthedocs.io/en/latest/rules.html#module-yamllint.rules.document_start
https://yamllint.readthedocs.io/en/latest/rules.html#module-yamllint.rules.document_end

While it would be the most annoying, throwing an error on this would make the updates go faster, as opposed to generating a lot of warnings that will take the team a bit to uncover and update.

@vgrassia vgrassia disabled auto-merge October 16, 2024 19:32
@vgrassia
Copy link
Member Author

@Eeebru created a card for this. I'll be holding on merging this until we clean up all the current workflows.

@vgrassia vgrassia self-assigned this Oct 16, 2024
@vgrassia vgrassia added hold and removed hold labels Oct 16, 2024
@vgrassia vgrassia enabled auto-merge (squash) October 21, 2024 18:44
@vgrassia vgrassia merged commit 7f3cf8e into main Oct 21, 2024
6 checks passed
@vgrassia vgrassia deleted the bre-370_forbid-document-start-marker branch October 21, 2024 20:57
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