Workflow linting and test separation #3684

withinfocus · 2024-01-18T18:12:00Z

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [X] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Performs a number of cleanup tasks on wording and lint for YAML, and moves tests around for better alignment and organization.

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

codecov · 2024-01-18T18:18:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aeca172) 32.42% compared to head (6524c55) 32.58%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3684      +/-   ##
==========================================
+ Coverage   32.42%   32.58%   +0.15%     
==========================================
  Files        1212     1212              
  Lines       63353    63353              
  Branches     4762     4759       -3     
==========================================
+ Hits        20542    20641      +99     
+ Misses      41766    41752      -14     
+ Partials     1045      960      -85

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

audreyality

⛏ - for minor or nitpick changes
❓ - for questions
💭 - for open inquiry
🎨 - for suggestions / improvements
⚠️ - for concerns needing attention
🌱 - for future improvements

.github/workflows/_move_finalization_db_scripts.yml

audreyality · 2024-01-18T22:04:05Z

.github/workflows/build.yml

+      - "main"
+      - "rc"
+      - "hotfix-rc"
+  pull_request:


❓ The way I read this is that you want to build on any push to main/rc/hotfix-rc, and all pull requests? The main thing I'm concerned about is that with nothing listed under pull_requests, that it's not clear PR branches will be built. Assuming I read it correctly, a comment indicating the behavior being relied upon here would be nice.

That's correct, that an open-ended trigger applies to all of that type.

audreyality · 2024-01-18T22:05:48Z

.github/workflows/build.yml

        uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608 # v4.1.0

-      - name: Set up dotnet
+      - name: Set up .NET


⛏️ I'd keep this dotnet to keep it consistent with the CLI command run on line 43.

.github/workflows/build.yml

audreyality · 2024-01-18T22:09:03Z

.github/workflows/build.yml

@@ -445,7 +402,7 @@ jobs:
          if-no-files-found: error

  build-mssqlmigratorutility:
-    name: Build MsSqlMigratorUtility
+    name: Build MSSQL migrator utility


⛏️ Here's another case where I think it's better to stick with the package name being built/installed. Or, barring that, fully spell out Microsoft SQL Server.

There's a few places where "MSSQL" is used and this is traditionally the shorthand and image name, at least in a lot of places.

audreyality · 2024-01-18T22:29:55Z

.github/workflows/test-database.yml

+          BW_TEST_DATABASES__1__CONNECTIONSTRING: "server=localhost;uid=root;pwd=SET_A_PASSWORD_HERE_123;database=vault_dev"
+          # Default Dapper SqlServer
+          BW_TEST_DATABASES__2__TYPE: "SqlServer"
+          BW_TEST_DATABASES__2__CONNECTIONSTRING: "Server=localhost;Database=vault_dev;User Id=SA;Password=SET_A_PASSWORD_HERE_123;Encrypt=True;TrustServerCertificate=True;"


⚠️ This should use the same connection info as ./Migrate.ps1.

Not sure what you're referring to here.

audreyality · 2024-01-18T22:32:34Z

.github/workflows/test-database.yml

+      - name: Docker Compose down
+        if: always()
+        working-directory: "dev"
+        run: docker compose down


❓ Is volume cleanup necessary?

audreyality · 2024-01-18T22:33:51Z

.github/workflows/test-database.yml

+          dotnet --info
+          nuget help | grep Version
+          echo "GitHub ref: $GITHUB_REF"
+          echo "GitHub event: $GITHUB_EVENT"


🎨 Since these are going to be seen by folks that might not know what they refer to, I'd be a bit more descriptive.

audreyality · 2024-01-18T22:36:06Z

.github/workflows/test.yml

@@ -0,0 +1,57 @@
+---
+name: Testing


⛏️ Maybe "dotnet" or ".Net" testing -- since the database tests are specific, this should be as well.

I kinda thought of this as the location for all logic-level and therefore "generic" tests, whereas the other file is specific infrastructure testing for a database.

audreyality · 2024-01-18T22:38:10Z

.github/workflows/test.yml

+jobs:
+  testing:
+    name: Run tests
+    runs-on: ubuntu-22.04


❓ Does this need to be tested on windows and *nix? I'm thinking mostly about path manipulation, fwiw, but it could also be important if we're running integration tests for IIS hosts.

.github/workflows/cleanup-after-pr.yml

withinfocus · 2024-01-19T16:18:15Z

Made a few more changes based on comments but left a few things alone, perhaps with some ambivalence around making the change or not and being careful about reaching too far here. My goal was almost entirely to tidy and better represent the test step, so I didn't want to make logic changes that materially affect our CI processes.

bitwarden-bot · 2024-01-23T16:42:12Z

Checkmarx One – Scan Summary & Details – aca2c71d-185f-4c5f-aea5-ff86496a8c50

New Issues

Issue	Source File / Package	Checkmarx Insight
Passwords And Secrets - Generic Password	/test-database.yml: 93	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 155	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 159	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 74	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 87	Query to find passwords and secrets in infrastructure code.
Unpinned Actions Full Length Commit SHA	/build.yml: 220	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...
Unpinned Actions Full Length Commit SHA	/build.yml: 578	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...
Unpinned Actions Full Length Commit SHA	/_move_finalization_db_scripts.yml: 27	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...
Unpinned Actions Full Length Commit SHA	/build.yml: 499	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...
Unpinned Actions Full Length Commit SHA	/_move_finalization_db_scripts.yml: 103	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...
Unpinned Actions Full Length Commit SHA	/build.yml: 466	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
	Passwords And Secrets - Generic Password	/infrastructure-tests.yml: 91
	Passwords And Secrets - Generic Password	/infrastructure-tests.yml: 78
	Passwords And Secrets - Generic Password	/infrastructure-tests.yml: 97
	Passwords And Secrets - Generic Password	/database.yml: 65
	Passwords And Secrets - Generic Password	/database.yml: 69
	Unpinned Actions Full Length Commit SHA	/_move_finalization_db_scripts.yml: 29
	Unpinned Actions Full Length Commit SHA	/build.yml: 622
	Unpinned Actions Full Length Commit SHA	/build.yml: 542
	Unpinned Actions Full Length Commit SHA	/_move_finalization_db_scripts.yml: 105
	Unpinned Actions Full Length Commit SHA	/build.yml: 509
	Unpinned Actions Full Length Commit SHA	/build.yml: 263

Workflow linting and test separation

2b5d6ea

withinfocus added 3 commits January 18, 2024 15:48

Name linting step

eb87471

Few more renames

0958414

Database testing consolidation

402d379

withinfocus marked this pull request as ready for review January 18, 2024 21:54

withinfocus requested a review from a team as a code owner January 18, 2024 21:54

audreyality reviewed Jan 18, 2024

View reviewed changes

mimartin12 reviewed Jan 18, 2024

View reviewed changes

.github/workflows/cleanup-after-pr.yml Outdated Show resolved Hide resolved

.github/workflows/cleanup-after-pr.yml Outdated Show resolved Hide resolved

withinfocus mentioned this pull request Jan 19, 2024

[PM-5052] Upgrade to .NET 8 #3461

Merged

withinfocus added 2 commits January 19, 2024 11:16

Few more renames and tweaks

97b81df

Merge branch 'main' into workflow-lint

6b41c28

withinfocus requested a review from mimartin12 January 19, 2024 16:27

Merge branch 'main' into workflow-lint

6524c55

vgrassia approved these changes Jan 23, 2024

View reviewed changes

mimartin12 approved these changes Jan 23, 2024

View reviewed changes

withinfocus merged commit c63db73 into main Jan 23, 2024
48 checks passed

withinfocus deleted the workflow-lint branch January 23, 2024 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workflow linting and test separation #3684

Workflow linting and test separation #3684

withinfocus commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

audreyality left a comment

audreyality Jan 18, 2024

withinfocus Jan 19, 2024

audreyality Jan 18, 2024

audreyality Jan 18, 2024

withinfocus Jan 19, 2024

audreyality Jan 18, 2024

withinfocus Jan 19, 2024

audreyality Jan 18, 2024

audreyality Jan 18, 2024

audreyality Jan 18, 2024

withinfocus Jan 19, 2024

audreyality Jan 18, 2024

withinfocus commented Jan 19, 2024

bitwarden-bot commented Jan 23, 2024

Workflow linting and test separation #3684

Workflow linting and test separation #3684

Conversation

withinfocus commented Jan 18, 2024 • edited Loading

Type of change

Objective

Before you submit

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

audreyality 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

withinfocus commented Jan 19, 2024

bitwarden-bot commented Jan 23, 2024

New Issues

Fixed Issues

withinfocus commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading