chore: extract dir paths to common vars

[dhaustein]

• centralized directory variables (FRONTEND_DIR, LOGS_DIR, COVERAGE_DIR, SCREENSHOTS_DIR, JUNIT_DIR) at pipeline level
  • replaced hardcoded "frontend" dir paths with ${FRONTEND_DIR} variable throughout
  • changed the npm commands to use --prefix instead of cd commands
• updated the Deno image from debian-1.46.3 to debian-2.0.2 for the licenseCheck job
• update the licenseCheck script to not be hardcoded to use only current directory
• added license-checker-rseidelsohn to deno.json (not sure if strictly needed but I saw some import errors without it)
• removed archiving frontend/logs from the test:frontend:unit job

[mender-test-bot]

There was an error running your pipeline, see logs for details.

[dhaustein]

@mender-test-bot sync

[dhaustein]

trying to re-open the PR

[mender-test-bot]

There was an error running your pipeline, see logs for details.

[dhaustein]

@mender-test-bot sync

[alfrunes]

CI change looks reasonable to me. However, I'm not too familiar with the npm setup here and would like to defer the approval to @mendersoftware/frontend-dependabot-reviewers

[mzedel]

Quite a few changes in a single commit that don't align with the commit message...

The change in the commit title is IMO a personal preference over the repo structure, which is indeed hardcoded. Can you give an explanation as to the improvements this would bring over the explicit reliance on the repo structure - also in light of the added indirection?

More general:
Pure formatting changes are IMO also mostly a personal preference and should go at least into a separate commit if they are included at all as they will usually just cause noise for the people editing the files frequently.

[mzedel]

Instead of using the independent deno config like this (outside of the licenseCheck script hierarchy), you could move the licenseCheck to the scripts, define the execution as a new task in there and make sure the comparison in the file points to the license csv file. The change in versioning requires the config file, as otherwise the missing "nodeModulesDir": "auto", would prevent retrieving the package in CI - this also causes the import map entry in there that you noted about 👌.

This wasn't intended to be in there, but would make a nice consolidation 👍.

[dhaustein]

We should address this in a followup PR, I am lumping too much stuff into this one PR as it is

[mzedel]

Sure, that would be a good practice. So remove the related changes from this PR as it conflates both and create a followup PR or task. Or separate the different aspects in separate commits as I requested.

[mzedel]

@dhaustein applying your own logic here: the contents of the script are frontend team code and as such the final say is: that change is poor, please adjust or create a follow up task.

[dhaustein]

we will be defining a QA role and who is responsible for what exactly, hopefully next week, until then we have the same role

[mzedel]

Interesting...
Until then we still have this PR opened before the fact and as the person mainly responsible for the code in here, I still insist on this being addressed before this can get merged or put into a follow up task, since the change as is, is considered bad practice and thus nothing we want in the codebase.

[alfrunes]

Just a few observation. Sorry I unsubscribed from this issue due to lack of frontend expertise.
+1 for removing the job-local directories
I'm not sure what would be the best approach for the number of new variables introduced, but I would personally prefer keeping it to a minimum.
It would also be great if you could split the different changes into separate commits that more accurately describe the changes.

[alfrunes]

How about instead of defining 5 different variables that points to frontend artifacts, consolidate everything into a FRONTEND_ARTIFACTS_DIR pointing to the root of all artifacts and update the jobs the scripts that generate these artifacts to write to sub-directories inside this path?
In that case it doesn't matter what you define the variable as and there will be less boilerplate dealing with the correct environment variables.

Suggested change

	FRONTEND_DIR: ${CI_PROJECT_DIR}/frontend
	LOGS_DIR: ${CI_PROJECT_DIR}/logs
	COVERAGE_DIR: ${FRONTEND_DIR}/coverage
	SCREENSHOTS_DIR: ${FRONTEND_DIR}/screenshots
	JUNIT_DIR: ${FRONTEND_DIR}
	FRONTEND_ARTIFACTS_ROOT: "${CI_PROJECT_DIR:-$(git rev-parse --show-toplevel)}/frontend/artifacts"

[dhaustein]

I like this approach!

It does rely on git, and I am not sure how it's going to work for things like shallow clones, but I also cannot see the $CI_PROJECT_DIR to not have a value. Something must've gone very wrong if that happened. Either way, a cool fallback.

[dhaustein]

I looked into the artifacts root path changes and it won't be as straightforward.

I'd need to add something akin to path.join(frontendDir, 'artifacts'); into vitest.config.ts and playwright.config.ts because they are set to output to their root which would be FRONTEND_DIR and not FRONTEND_DIR/artifacts

I'd rather not change it this way, unless we really really want it.

I can go back to a partial approach. Keep the FRONTEND_DIR variable

variables:
  FRONTEND_DIR: ${CI_PROJECT_DIR:-$(git rev-parse --show-toplevel)}/frontend

but don't do anything extra for the different artifacts dirs. And for the job artifacts just do:

artifacts:
    paths:
      - ${FRONTEND_DIR}/coverage
      - ${FRONTEND_DIR}/screenshots
      - ${FRONTEND_DIR}/junit

What do you think?

[dhaustein]

Actually turns out that

FRONTEND_DIR: ${CI_PROJECT_DIR:-$(git rev-parse --show-toplevel)}/frontend

won't work because gitlab will not do command substitution, so I changed it to simply

FRONTEND_DIR: ${CI_PROJECT_DIR}/frontend

[dhaustein]

@alfrunes What's your take on he changes now?

[dhaustein]

Hi @kjaskiewiczz @merlin-northern as discussed on the server meeting, I've broken down these changes into hopefully cleaner commit structure.

[vpodzime]

The feat commits are misleading here -- the changes are not really features of mender-server. It would be OK if this was a tests repository, but we don't want to present these changes are new features in Mender Server, right?

Looks good to me otherwise.

[merlin-northern]

okey so I do not want to attack (too much) you, but this couple of lines is my clipboard, so I come here to copy from when I forget how to run the ci for the Frontend. and if I have to remove that many chars from the command, I will have rather re-typed it. so all in all I do not really see the benefit of the FRONTEND_DIR variable. why did you introduce it?

[dhaustein]

Doing a cd dir in a ci is considered an anti-pattern, it's moving the pointer around and all the scripts assume a certain directory structure. Luckily all the tooling has some way to force a "workdir" so it can be defined once and re-used. Also when working on the CI you have to keep that in your head too.

If you rely on the CI code for your local needs, the code will still work the same, you just need to ensure you have the new env var.

And if you are copy pasting it every time, maybe it's time to create a script. 😁

[mzedel]

@dhaustein your question didn't answer the question to the why of the variable, but a why to the (undisputed) --prefix. Please come up with a reasonable explanation to the why of the variable.

As to best practices, it is also a best practice to reduce complexity where possible - in comparison a single level folder definition that is not only shorter than the variable definition, but moreover won't ever be an issue getting inherited in child pipelines/ expanded in a script/ is clearly readable without knowing the full pipeline definition.

And since this is repeatedly ignored here: the CI instructions are not only used by us internally and if we want to add the extra indirection we should have a good reason - so far I haven't seen any.

[dhaustein]

I believe the why has been discussed at least thrice now.

This is decreasing complexity, like I said to a comment to Peter, please read it.

I agree that in an ideal world the CI and the local would work the same. Alas we are not there yet and probably never will be 100%. Copying over lines from a CI script so you can run stuff locally, instead of relying on, I dunno, makefiles? is also a symptom of bad design.

Additionally, to this. Since I am in charge of the test automation, I have the final say. If you have a problem with this, talk to Marcin or Thomas.

[mzedel]

@dhaustein you should re-read the company handbook on the decision making process and I'm talking with both to hopefully prevent more of these discussions with you.

So how can an added layer of indirection decrease complexity? Now you have to do a lookup of: script => line => variable => line -- in comparison to: script => line ... we clearly must have a diverging counting system. So no, your why has not been discussed in a convincing way.
And again it's to avoid adding steps also to external consumers of the CI file - adding additional dependencies (such as make) for them to build parts of the application is in contrast to this.

[dhaustein]

@merlin-northern I could add the new env var to https://github.com/mendersoftware/mender-server/blob/main/.env And if you use the likes of direnv, you would be able to copy and use the CI commands withouth having to think about the env var. Would that work dor you?

[dhaustein]

The feat commits are misleading here -- the changes are not really features of mender-server. It would be OK if this was a tests repository, but we don't want to present these changes are new features in Mender Server, right?

Looks good to me otherwise.

@vpodzime Yeah I wasn't sure how this team approaches features in and around testing. I will reword the commits.

[kjaskiewiczz]

I'm not an expert here and I don't have a strong opinion.
For sure I don't like the temperature of the discussion. Is the change worth it? Are we only discussing frontend vs ${FRONTEND_DIR}?

If I understand correctly @dhaustein wants to standardize things, which is good, and @mzedel is trying to avoid issues, which is also good.

As for the arguments:

mitigating bad practice of hardcoding the same values all over and extracting them into one single source of truth, this is programming 101

In programming I think this would be a good case for using const, but there are no consts in gitlab. Still I get the idea of having it organized this way.

Indeed having this in a variable is what's the risk and we can avoid them from showing up as configuration by not introducing the option - especially as it's not a nested directory as the e2e tests dir would be. My bigger gripe is that the value isn't debatable - even if you set FRONTEND_DIR instead of frontend - it will have to be correctly spelled and that's it.

I agree that if we see the risk in the change and we've seen some issues caused by similar approach in the past I would skip this change.

So as for this particular change/commit +1 for replacing cd with prefix and -1 for replacing fronted with ${FRONTEND_DIR}, but as I said I'm not an expert here and I wouldn't relay on my opinion when making final decision.

[oldgiova]

My two cents here; as for my experience when testing CI pipelines locally, I found very valuable the direnv project: by having a .envrc with:

export FRONTEND=frontend

would solve both issues: you can run the same code from your terminal as in CI, and you can keep Gitlab best practices.

This makes direnv as a requirements for the project, though.. Fell free to ignore my 2 cents

[mender-test-bot]

Merging these commits will result in the following changelog entries:

Changelogs

mender-server (extract-paths-to-dirs)

New changes in mender-server since main:

• Centralize license-checker-rseidelsohn dependency in Deno imports
• Removed not needed SERVER_ROOT and GUI_REPOSITORY env vars supplied to the frontend/tests/e2e_tests/run script (the script exports the vars on its own)
• Removed unused steps for making and archiving the frontend/logs dir
• Updated the Deno image from debian-1.46.3 to debian-2.0.2 for the licenseCheck CI job
• Add support for specifying root directory for checkGeneratedLicenses() in licenseCheck script
• Refactored the frontend pipeline to use a centralized FRONTEND_DIR variable instead of hardcoded 'frontend' paths and 'cd frontend' commands

[mzedel]

@oldgiova s mention of

Gitlab best practices.

made me think, maybe there is guidance from them on their CI - and turns out it's even better: their CI is of course open source. So you can go straight to their frontend file for an example on how they consider their CI to be used and they even share the same frontend name for their frontend things: https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/ci/frontend.gitlab-ci.yml?ref_type=heads#L347 - turns out they use variables for things to be reassigned during the pipeline.
So keeping the paths seems to align with the tool standard and is arguably considered best practice there.

I highly recommend anyone interested, but @dhaustein in particular to take a look through the main gitlab CI file and the corresponding includes folder.

[dhaustein]

@oldgiova s mention of

Gitlab best practices.

made me think, maybe there is guidance from them on their CI - and turns out it's even better: ...

But that is exactly what I am trying to do with

FRONTEND_DIR: ${CI_PROJECT_DIR}/frontend

[mzedel]

putting it into a picture to avoid confusion, but how is this:

relying on an env var for the path in the way that this is:

FRONTEND_DIR: ${CI_PROJECT_DIR}/frontend

[dhaustein]

@mzedel

relying on an env var for the path in the way that this is:

FRONTEND_DIR: ${CI_PROJECT_DIR}/frontend

I thought you were referring to the likes of

RSPEC_PROFILING_FOLDER_PATH: rspec/profiling

or the other *_PATH vars which are then re-used throughout.

The primary change in this PR is me trying to extract the repeated pattern of ${CI_PROJECT_DIR}/frontend and in doing so get rid of cd frontend/ I actually don't care if this is an env var in the end or not, but Gitlab doesn't let me have too many choices here in how to do it cleanly.

If your problem is that this would create a new variable, we can go with something like:

.frontend-dir-var: &frontend-dir-var
  FRONTEND_DIR: ${CI_PROJECT_DIR}/frontend

Which is a style Gitlab's CI code uses, too. However, this would mean having an instance of:

variables:
  <<: *frontend-dir-var

in every job that wants to use this var again.

[mzedel]

@dhaustein look again please.
The paths defined in variables are used in CI and not ones reliant on the project structure. The RSPEC_PROFILING_FOLDER_PATH is where profiling results gathered from CI runs go, something like GLCI_ASSETS_TAG_ENV_FILE_PATH that is generated during a CI job, temp paths like WEBPACK_COMPILE_LOG_PATH - OTOH static paths that exist in the repo are referenced in full.

It's quite consistent among the files (e.g. https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/ci/gitlab-gems.gitlab-ci.yml and the 15 repetitions of the .gitlab/ci/templates/ folder, or the countless references to the script folder in all of the files, ...) so I would assume it's a common practice used by the people actually developing the tool - so likely recommended to stick to it.

[dhaustein]

@mzedel Yes, however the usage of variables in includes is limited https://docs.gitlab.com/ci/yaml/includes/#use-variables-with-include

They cannot replace the common filepath with a var, not within the job at least, they'd have to define it in the global CI/CD settings which would hide the value to a completely different place.

On our end for example - our yocto pipeline hinges on $WORKSPACE for similar reasons. I went over the history in mender-qa and clearly a lot of thought went into its design even before it was ported from Jenkins.

The thing is complex sure, but for one the usage of $WORKSPACE makes it, at least, easier to reason about where the commands are running. I want something like that here too.

I know DRY is not a dogma, but generally a good idea. What should I change in this regard so that you are more receptive to these changes?

[mzedel]

To the handling in the yocto pipeline... I think that's a very, very different setup, as the client pipeline relies on multiple repositories (getting dependencies as submodules or cloning them, building them inside and going back again) and at least I haven't touched the build steps of any of this (as personally I found both the Jenkins setup & the gitlab setup not at all clear to work in + I lack the client knowledge to make informed decisions there). I'd be happy to have a chat about the reasons for the switch from Jenkins in the first place & things we considered - maybe we can do this once Lluis is back..?
The server side OTOH was adjusted quite a bit to be contained in the repositories it used to live in, so the $WORKSPACE situation is intentionally not happening. Any of the consumed dependencies should be built externally - if we still have some, these need to be addressed and externalized IMO.

So my recommendation would rather be to not make steps to the (afaict more Jenkins-aligned) mender-qa setup, but instead decouple mender-qa if possible & align with Gitlab built in features to no longer need the $WORKSPACE workaround (IMO that should be handled differently to rely on CI_PROJECT_DIR, but again I never dared to touch this, so I'm judging this from a weak knowledge base).

As to the PR here: I think the removal of cds + fixes to existing vars are great & I'd be happy to provide the changes to the licenseCheck to make this run under the deno tasks file (+ I'll adjust the env reliance for the e2e tests separately as the GUI_REPOSITORY really looks off, coming from the pre-monorepo days). The indirection over the dir variable OTOH is however something I think is risky without an added benefit due to the nature of the value the var replaces.
As to the repo here: the feature based/ PR based deployments would be amazing to have, improving parallelization of the jobs especially in the full pipeline as well, caching!, aligning rules etc. also - I think there are a lot of possibilities...

Regarding what's in the gitlab repo:

@mzedel Yes, however the usage of variables in includes is limited https://docs.gitlab.com/ci/yaml/includes/#use-variables-with-include

That would address the gitlab-gems case, but a look through the gitlab codebase shows that there is no repository path mapped through a variable (ok... there is one exception I found not in CI, but in a template: https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Pages/SwaggerUI.gitlab-ci.yml?plain=1#L29).

[pasinskim]

Thanks all. @mzedel @dhaustein let's land the clear wins and pause the contentious part.

@dhaustein the DRY principle is a guideline, not an absolute law. It is most useful when abstracting values that might change. The name of the main frontend source directory is highly unlikely to change. In this context, the explicit path frontend/ is clearer and more direct than the variable $FRONTEND_DIR. Adding the variable makes the configuration more complex to read and debug for no tangible gain. It also make sense to follow the tool's own developers (Gitlab) practices. Other changes, like replacing cd with npm --prefix, are IMO legitimate improvements.

What I propose is:

• keep switching from cd to --prefix
• keep the Deno/license tweaks and logs cleanup
• revert FRONTEND_DIR; use literal frontend/… paths

Also, please keep the discussion to the technical arguments here and other things (process/ownership) let's discuss elsewhere.