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

ci: add treefmt as code formatting multiplexer, refactor CI to avoid duplication, reorg CI into DevOps workflow #4219

Merged
merged 123 commits into from
Oct 14, 2024

Conversation

JakobLichterfeld
Copy link
Collaborator

@JakobLichterfeld JakobLichterfeld commented Sep 18, 2024

As we do have a multitude of programming languages, running each linter separate is not efficient.

Treefmt solves this and comes with speed improvements as well. See the documentation for more information.

  • treefmt.toml config
  • lint whole project accordingly
    • exclude Grafana Dashboards
  • CI
    • refactor CI to avoid duplication
    • reorg CI into DevOps workflow
    • allow ghcr_build parallel to elixir test
    • GitHub Action for treefmt or Pre-Commit Hook using treefmt-nix

closes #4218

Copy link

netlify bot commented Sep 18, 2024

Deploy Preview for teslamate ready!

Name Link
🔨 Latest commit b48ac45
🔍 Latest deploy log https://app.netlify.com/sites/teslamate/deploys/670b9662a55fee0008fafd51
😎 Deploy Preview https://deploy-preview-4219--teslamate.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JakobLichterfeld JakobLichterfeld added enhancement New feature or request kind:idea Idea for new feature or some form of enhancement github_actions Pull requests that update Github_actions code labels Sep 18, 2024
@swiffer
Copy link
Contributor

swiffer commented Sep 18, 2024

👍, but would prefer formatting as created by grafana export for dashboards

@JakobLichterfeld
Copy link
Collaborator Author

👍, but would prefer formatting as created by grafana export for dashboards

Yes, I stumbled over that too. Will exclude the dashboard config from the prettier linter.

@JakobLichterfeld
Copy link
Collaborator Author

Will exclude the dashboard config from the prettier linter.

Done

@JakobLichterfeld JakobLichterfeld changed the title ci: add treefmt as code formatting multiplexer ci: add treefmt as code formatting multiplexer, refactor CI to avoid duplication, reorg into DevOps workflow Sep 19, 2024
@JakobLichterfeld JakobLichterfeld changed the title ci: add treefmt as code formatting multiplexer, refactor CI to avoid duplication, reorg into DevOps workflow ci: add treefmt as code formatting multiplexer, refactor CI to avoid duplication, reorg CI into DevOps workflow Sep 19, 2024
@JakobLichterfeld
Copy link
Collaborator Author

JakobLichterfeld commented Oct 9, 2024

CI ran successful on push but not on pull_request_target.

…nvalid characters to avoid invalid reference format"

This reverts commit 02abb03.
- Ensure workflow only run if there are no changes to the .github folder
- Allow workflow to run on workflow call or PRs from forks
- Prevent duplicate runs for PRs from non-forks
- Avoid invalid reference format for cache name in PRs from our repository
@JakobLichterfeld
Copy link
Collaborator Author

I will ensure pull_request_target only runs on real forks to avoid invalid cache name and unnecessary duplicate runs.

@brianmay
Copy link
Collaborator

brianmay commented Oct 9, 2024

I think the mix hash needs to be updated also:

error: hash mismatch in fixed-output derivation '/nix/store/gly3c0rmyx308dzyl4qssf0irh175wgh-teslamate-mix-deps-1.30.2-dev.drv':
         specified: sha256-ukE7PKrsRaw6TWDSqTJljsPJQedi39mZgGFrMINEJlw=
            got:    sha256-Y+CGgvnSCiiuyhtsQ+j0vayq1IHO5IEPVl+V/wwTd6w=
error: 1 dependencies of derivation '/nix/store/1ladsmz0i5x6zzx15bqqpjv2fywa746g-teslamate-1.30.2-dev.drv' failed to build

Copy link
Collaborator

@brianmay brianmay left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, just needs the two issues resolved.

Copy link
Contributor

@scottbot95 scottbot95 left a comment

Choose a reason for hiding this comment

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

So I did a little digging because it was bothering me that we have to copy the treefmt config in two places and I discovered a potential workaround. treefmt.settings is just used to generate treefmt.build.configFile and treefmt.build.configFile is what is actually used as the config file for the treefmt executable. So we could simply set treefmt.build.configFile ourselves with lib.mkForce and point it to the treefmt.toml in repo. That said, from their code it's unclear to me whether this is an intended extension point or merely an implementation detail so I'm not sure if we really want to rely on it. It's probably fine... What's the worst that could happen 😅

.github/workflows/ensure_linting.yml Show resolved Hide resolved
nix/flake-modules/formatter.nix Outdated Show resolved Hide resolved
@JakobLichterfeld
Copy link
Collaborator Author

That said, from their code it's unclear to me whether this is an intended extension point or merely an implementation detail so I'm not sure if we really want to rely on it. It's probably fine... What's the worst that could happen 😅

Yeah, I found that hole in the fence too, but was too excited when I finally got it working to do another refactoring :-)

@brianmay
Copy link
Collaborator

This error was a bit unexpected:

$ nix flake check --override-input devenv-root "file+file://"<(printf %s "$PWD") .
warning: Git tree '/home/brian/tree/3rdparty/teslamate' is dirty
warning: not writing modified lock file of flake 'git+file:///home/brian/tree/3rdparty/teslamate':
• Updated input 'devenv-root':
    'file:///dev/null?narHash=sha256-d6xi4mKdjkX2JFicDIv5niSzpyI0m/Hnm8GGAIU04kY%3D'
  → 'file:///proc/self/fd/11?narHash=sha256-MR36Ywqs/zTxXcyqXW6MESFZ3X7OT91FF2wTpsZPoEg%3D'
error:
       … while checking flake output 'checks'

         at «none»:0: (source not available)

       … while checking the derivation 'checks.x86_64-linux.default'

         at «none»:0: (source not available)

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: attribute 'nixosModules' missing

       at /nix/store/ngxcbaks38d1vpxflw88983n205bkfic-source/flake.nix:227:33:

          226|                     nixosModules.default = import ./module.nix { inherit self'; };
          227|                     imports = [ self'.nixosModules.default ];
             |                                 ^
          228|                     services.teslamate = {

flake.nix Outdated Show resolved Hide resolved
@JakobLichterfeld
Copy link
Collaborator Author

JakobLichterfeld commented Oct 13, 2024

Not sure yet what is happening with the CI. Elixir test fails even if all test cases pass. I assume this is due to the postgres17 update and was not found with CI before, as the build caching was used in the postgres17 pr.

Edit: Wow, GitHub Runners are really slow today. Will investigate further next week.

@brianmay
Copy link
Collaborator

brianmay commented Oct 13, 2024

Tests "succeeded" now, after I clicked retry.

Actually if you look at the logs, there was one scary looking error that isn't generating a test failure:

** (FunctionClauseError) no function clause matching in DateTime.to_naive/1
    (elixir 1.16.2) lib/calendar/datetime.ex:925: DateTime.to_naive({:ok, ~U[2003-07-01 00:00:00Z], 0})
    (teslamate 1.30.2-dev) lib/teslamate/log.ex:185: TeslaMate.Log.get_positions_without_elevation/2
    (teslamate 1.30.2-dev) lib/teslamate/terrain.ex:77: TeslaMate.Terrain.handle_event/4
    (stdlib 5.2.3.2) gen_statem.erl:1397: :gen_statem.loop_state_callback/11
    (stdlib 5.2.3.2) proc_lib.erl:241: :proc_lib.init_p_do_apply/3

relevant code is at lib/teslamate/log.ex:185 is:

    date_earliest =
      cond do
        min_id == 0 ->
          DateTime.add(DateTime.utc_now(), -10, :day)

        true ->
          DateTime.from_iso8601("2003-07-01T00:00:00Z")
      end

    naive_date_earliest = DateTime.to_naive(date_earliest)

Problem is DateTime.from_iso8601 returns a tuple, and could in theory fail. Sigh. Wish Elixir had strong type checking...

In actual practice, this probably function I think can't fail, the value is constant.

@brianmay
Copy link
Collaborator

I think the above is not a regression in this pull request, so opened a new issue. #4260. Probably better to merge it separately so it doesn't get lost.

Think it is safe to merge this PR now...

Copy link
Collaborator

@brianmay brianmay 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 now. Minor stuff can be cleaned up as we find it after this is merged.

Copy link
Contributor

@scottbot95 scottbot95 left a comment

Choose a reason for hiding this comment

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

Also looks good to me. I'll have a PR before to long to do some further clean-up on the nix code.

@JakobLichterfeld
Copy link
Collaborator Author

Tests "succeeded" now, after I clicked retry.

Yeah, GitHub Runners have been slow and I assume a timing error.

Actually if you look at the logs, there was one scary looking error that isn't generating a test failure:

Saw it and was confused as to why there was no failure.

Wish Elixir had strong type checking...

That explains it.

I think the above is not a regression in this pull request, so opened a new issue. #4260. Probably better to merge it separately so it doesn't get lost.

Ty!

@JakobLichterfeld JakobLichterfeld merged commit fdf8185 into master Oct 14, 2024
19 checks passed
@JakobLichterfeld JakobLichterfeld deleted the treefmt branch October 14, 2024 06:54
JakobLichterfeld added a commit that referenced this pull request Oct 19, 2024
…duplication, reorg CI into DevOps workflow (#4219)

* ci: add treefmt as code formatting multiplexer

* style: linter findings in entrypoint.sh script

* style: linter findings for yaml and yml

* style: linter findings for json files

* style: linter findings for nix files

* style: linter findings for js files

* style: linter findings for dashboards.sh

* style: linter findings for md and mdx files

* chore: remove unused clang formatter in treefmt config

* style: linter findings for mdx files

* ci: exclude Grafana dashboard JSON files from prettier formatting

* Revert "style: linter findings for json files"

This reverts commit f40c2e1.

* ci: exclude Grafana dashboard JSON files from all formatting as we use the grafana export style

* style: linter findings for json files

* doc: update changelog

* ci(refactor): use composite action to avoid duplication in elixir workflow

* doc: update changelog

* ci: prevent workflow runs for certain conditions and allow scheduled runs

* ci(refactor): use reusable workflow to check paths

* ci(fix): correct output syntax for check_paths workflow and setting base branch

* ci(refactor): use reusable workflows for streamlined DevOps pipeline

* ci(fix): add write permission for packages in DevOps workflow

* ci(test): test DevOps workflow

* ci(test): test DevOps workflow

* ci(fix): Update condition for spell_check, ensure_linting, elixir, and ghcr_build workflows to reflect empty result instead of false

* ci: revert test DevOps

* ci(refactor): allow ghcr_build parallel to elixir test

* ci(refactor): Remove redundant check_paths job from elixir.yml, elixir_test.yml, and spell_check.yml workflows, check is done in devops.yml

* feat: add treefmt-nix to nix flake (#4219 - @JakobLichterfeld)

* ci: ensure proper linting via treefmt

* ci(test): test ensure_linting workflow

* ci(fix): checkout code for spell_checker to access file to check

* ci(fix): allow impure in ensure_linting workflow

* Revert "ci(test): test ensure_linting workflow"

This reverts commit a67b17e.

* ci(fix): correct use of flake-utils for formatter and checks

Co-authored-by: scottbot95 <scottbot95@gmail.com>

* ci(fix): correct use of flake-utils for treefmt

Co-authored-by: scottbot95 <scottbot95@gmail.com>

* refactor: Remove unnecessary imports in flake

* ci(fix): correct syntax in flake

* ci(refactor): Remove unused code in flake.nix

* style: standardised style for input url in flake

* ci(fix): treefmt-nix config with existing options

* ci(feat): Add Nix binary cache and update treefmt command in CI workflow

* ci(refactor): Remove unused code in flake.nix

* fix: include devShell packages only on supported platforms

* fix: update hash for mix-deps package in flake.nix

* ci(fix): Update treefmt command in CI workflow

* ci(test): test ensure_linting workflow

* feat: ensure mix deps are present in devShell

* ci(feat): use flake-parts to enable treefmt-nix

* feat: use flake-parts

* fix: correct use of flake-parts for package build

* doc: update CI badge URL for devops workflow

* ci(fix): handle empty path filter output

* ci: remove --impure flag from treefmt command in CI workflow

* fix: correct treefmt.config settings in formatter.nix

* fix: correct flake-parts inputs, avoid with in imports

* fix: correct program name for mix-format in formatter.nix

* feat: devenv via flake-parts

* fix: correct use of legacy nix code with flake-parts

* ci(fix): correct nix develop command in ensure_linting.yml

* refactor: list imports explicitly in flake, rename folder to flake-modules to be precise

* style: use tabs for indent size to format sh

* style: use nixfmt-rfc-style

* Revert "style: use nixfmt-rfc-style"

This reverts commit 0820561.

* style: use nixfmt style

* fix: remove glibcLocales from optional dependencies to avoid  "A definition for option `packages."[definition 4-entry 16]"' is not of type `package'."

* fix: remove inotify-tools from optional dependencies to avoid  "A definition for option `packages."[definition 4-entry 16]"' is not of type `package'."

* fix: Remove inotify-tools and glibcLocales from optional dependencie

* fix: correct file paths in flake.nix to version

* fix: add ELIXIR_ERL_OPTIONS to shell environment to force utf8 locale

* fix: add LOCALE_ARCHIVE to shell environment in flake.nix

* Revert "fix: add LOCALE_ARCHIVE to shell environment in flake.nix"

This reverts commit d45f6e3.

* ci(refactor): rename workflow to elixir_dep_verification_and_static_analysis.yml to better reflect the intention,, remove duplicate checks

* ci(debug): debug locale settings

* Revert "ci(debug): debug locale settings"

This reverts commit 9b402f3.

* Revert "fix: add ELIXIR_ERL_OPTIONS to shell environment to force utf8 locale"

This reverts commit d02419c.

* fix: add LOCALE_ARCHIVE to shell environment in flake.nix

* Revert "fix: add LOCALE_ARCHIVE to shell environment in flake.nix"

This reverts commit 761b437.

* fix: add LANG=C.UTF-8 to shell environment in flake.nix

* fix: add mix local.rebar and mix local.hex commands to flake.nix

* fix: pin devenv to version without unix socket bug

* chore: update nixpkgs to nixos-24.05 and update dependencies

* doc: add treefmt config comments

* ci: do not expose treefmt formatter programs in devshell

* fix: correct use of module option to enable PostgreSQL server in flake.nix

* Revert "chore: update nixpkgs to nixos-24.05 and update dependencies"

This reverts commit a6ea3f2.

* feat: consistent use of erlang 26 and elixir 1_16 in flake

* ci: switch to macOS runner for linting workflow

* Revert "ci: do not expose treefmt formatter programs in devshell"

This reverts commit 1ecfa45.

* Revert "ci: switch to macOS runner for linting workflow"

This reverts commit 7b43066.

* ci: Remove nixpkgs channel specification in ensure_linting workflow

* ci(debug): Add debug output for PATH and NIX_PATH in flake.nix

* Revert "ci(debug): Add debug output for PATH and NIX_PATH in flake.nix"

This reverts commit 07faec5.

* fix: avoid the need for impure for devenv

see #4245

* fix: remove invalid custom build.check for formatter and use default

* style: linter findings

* fix: Add emptyTest to avoid nix flake check test execution on non-Linux systems

* chore: Remove LANG=C.UTF-8 from enterShell in flake.nix

* ci(fix): Remove --impure flag from treefmt command in CI mode

* ci(fix): avoid impure mode in ensure_linting workflow

* style: linter findings

* ci(debug): debug elixir version and locale

* chore: Update flake.lock dependencies

* feat: use newer devenv as unix socket bug is fixed in upstream

cachix/devenv#1497

* fix: set rebar3 path in devenv

* Revert "ci(debug): debug elixir version and locale"

This reverts commit 7ecdc77.

* ci: re-enable path check in DevOps workflow

* doc: update Development and Contributing guide with nix and treefmt

* ci: use PostgreSQL 17

* style: linter findings

* ci(fix): ensure cache name in build action does not contain invalid characters to avoid invalid reference format

* doc: update changelog

* Revert "ci(fix): ensure cache name in build action does not contain invalid characters to avoid invalid reference format"

This reverts commit 02abb03.

* ci: remove branch restriction for check_paths workflow to increase sec

* ci(fix): run ghcr build workflow only for specific conditions

- Ensure workflow only run if there are no changes to the .github folder
- Allow workflow to run on workflow call or PRs from forks
- Prevent duplicate runs for PRs from non-forks
- Avoid invalid reference format for cache name in PRs from our repository

* doc: update changelog

* fix: update hash for mix-deps package in flake.nix

* fix: disable flakeCheck for formatter, as mix format need the dep to be fetched beforehand

* ci(fix): run ghcr build workflow only for specific conditions

* fix: move nixosModules.default to top-level attribute set

* refactor: remove unnecessary config nesting in formatter.nix

* ci(fix): ensure version for buildx is set to correct name

---------

Co-authored-by: scottbot95 <scottbot95@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request github_actions Pull requests that update Github_actions code kind:idea Idea for new feature or some form of enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ci: add treefmt as code formatting multiplexer
4 participants