Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

Add .gitlab-ci.yml-compatible continuous integration configuration #8316

Merged
merged 3 commits into from Nov 3, 2021
Merged

Add .gitlab-ci.yml-compatible continuous integration configuration #8316

merged 3 commits into from Nov 3, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jun 28, 2021

Summary

This changeset provides a GitLab-CI compatible YAML config to run linting and unit testing for mattermost-webapp.

The initially-presented filename .gitlab-ci.collabora.yml is what I've been using verbatim during development at @collabora - open to moving this to a contrib directory and/or otherwise reducing any namespace pollution or confusion.

As mentioned in mattermost/mattermost#17843, the CI_CONFIG_PATH can be used to configure GitLab CI to read from this configuration file on a per-repository basis.

Ticket Link

Relates to mattermost/mattermost#17843.

Release Note

This doesn't affect the functionality of the Mattermost product and application.

NONE

@mattermod
Copy link
Contributor

Hello @jayaddison-collabora,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@mattermod mattermod requested a review from Willyfrog June 28, 2021 17:19
@mattermod mattermod added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jun 28, 2021
@Willyfrog
Copy link
Contributor

hey, thanks for the PR, I'll assign some reviewers as I'm not familiar with gitlab's yml

@Willyfrog Willyfrog requested review from cpanato and metanerd June 28, 2021 17:35
Copy link
Contributor

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

agree in moving this to a contrib directory and not having it in the root.

Also will be good to have some documentation in how to use this

thanks for this PR!

@ghost
Copy link
Author

ghost commented Jun 29, 2021

Thanks @cpanato, I'll relocate the file soon. Would somewhere under https://github.com/mattermost/mattermost-developer-documentation/tree/master/site/content/contribute be a good place to offer some corresponding documentation?

@cpanato
Copy link
Contributor

cpanato commented Jun 29, 2021

@jayaddison-collabora I would say, this file should be in a directory maybe called contrib in this repo together with a README.md in the same directory to explain how to use that.

the documentation in the other repo will be good as well, but I would like to see something together with this addition

thanks!

@ghost
Copy link
Author

ghost commented Jun 29, 2021

Sounds good to me, thanks @cpanato!

Copy link
Contributor

@metanerd metanerd left a comment

Choose a reason for hiding this comment

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

Thank you @jayaddison-collabora for your contribution! Great suggestion!

@ghost
Copy link
Author

ghost commented Jul 1, 2021

Thanks @metanerd!

@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

@ghost
Copy link
Author

ghost commented Jul 12, 2021

There are at least two issues to address here I think:

  • [resolved] One is an immediate practical issue; the test build step has begun failing due to an upstream issue Full-icu tries to install a version of icu4c-data (68l) that doesn't exist nodejs/full-icu-npm#61 - I think it's best to resolve that by moving to a non-Alpine-based image, where a packaged distribution of ICU data is available (removing the need for the full-icu NPM package)

  • A larger structural issue is that these continuous integration steps could be quite wasteful if they pull full base images on each pipeline run. Building custom images and pushing them to a local registry (as the existing .gitlab-ci.yml does) is a nice efficient way to resolve this, although it makes sharing the build and test infrastructure more challenging. I feel like some of this is really an architecture issue with GitLab and container images more generally -- but in the short term, perhaps placing a needs dependency (or pipeline stage) between lint and test steps would at least mean that we don't duplicate network traffic and package installation for a pipeline that isn't going to succeed. Alternatively, we could perform both linting and unit testing within a single job.

@ghost
Copy link
Author

ghost commented Aug 3, 2021

Some more thoughts: although running linting and unit tests is a start, I think a more valuable follow-on from this would be to add the ability to produce build packages of mattermost-webapp and mattermost-server.

That would require some careful planning because the webapp dist package is included in the server package build. I think it'd be possible though: GitLab provides multi-project pipelines, so a change to either project could start a webapp build pipeline which in turn could communicate a cross-project artifact (NB: premium feature) and start a server build pipeline that includes that package.

A bonus would be to use the CI_COMMIT_TAG predefined variable and pass that through as the BUILD_NUMBER.

It's possible I might experiment with some of this soon, it'd make life easier in a bunch of ways.

@Willyfrog
Copy link
Contributor

friendly ping @cpanato

@ghost
Copy link
Author

ghost commented Oct 26, 2021

NB: This continuous integration config still works fine against the mainline branch of mattermost-webapp, but it seems to have issues with the release-6.0 branch.

The problem there may be something to do with formatjs, as pulled in by the react-intl dependency. It surfaces as errors that look like this:

Unexpected console logsError: [@formatjs/intl Error MISSING_DATA] Missing locale data for locale: "es" in Intl.NumberFormat. Using default locale: "es" as fallback. See https://formatjs.io/docs/react-intl#runtime-requirements for more details

A workaround is to specify NODE_ICU_DATA=node_modules/full-icu during the npm run test:speed. But that doesn't explain what introduced the problem in that branch. If this begins affecting mainline too, perhaps I'll push that workaround here too; it'd be nice to figure out the cause first though.

Update: sorry about the noise here, I think I'm mistaken; the cause of these formatjs errors (and the resulting test failures) looks more likely to be from locally-applied commit(s) that introduced some localizable components. I'm not hugely familiar with the internationalization processes in the Mattermost webapp, but probably should have guessed that could be the cause! I'll try to explain this more clearly once I understand the precise cause.

Edit: update dependency link to point to the most recent commit on release-6.0 instead of mainline, since the former branch is where the error was encountered

Edit: add explainer: it looks like these errors were a false alarm; they only occur in a local build environment with some customizations applied

@Willyfrog Willyfrog removed the 2: Dev Review Requires review by a core commiter label Oct 29, 2021
@Willyfrog
Copy link
Contributor

@cpanato does this need QA?

@cpanato
Copy link
Contributor

cpanato commented Oct 29, 2021

@cpanato does this need QA?

nope, this is just an example of a gitlab pipeline, nothing that we will run here

@Willyfrog Willyfrog removed the 3: QA Review Requires review by a QA tester label Nov 3, 2021
@Willyfrog Willyfrog added the 4: Reviews Complete All reviewers have approved the pull request label Nov 3, 2021
@Willyfrog Willyfrog merged commit 6ff28ce into mattermost:master Nov 3, 2021
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Nov 3, 2021
@ghost ghost deleted the contrib/gitlab-ci-linting-unit-testing branch November 3, 2021 17:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants