docs: contributing guidelines fix

[taddes]

Description

A community contributor recently mentioned a link was dead in our README file related to Mozilla best practices for contributing. Issue/Comment:

Dead link: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities

Given the contributing guidelines are no longer maintained and each project/team maintains its own contribution guidelines, this link should be removed and some updates made to the contribution documentation.

Issue(s)

Closes STOR-394.

[felfert]

I was that community contributor, so just a sidenote regarding:

"Your code must run and pass all the automated tests before you submit your PR"

If with "all the automated tests" you mean your circleCI, then this raises a chicken-egg problem, because
only after submitting a PR those get triggered. I would suggest changing that to:

"Your code must run and pass all the automated tests before your PR is considered for review"

Furthermore, your circleCI is set up in a way that all tests are failing, because of some missing cloud key.
It would be nice to have automatet tests run correctly for PRs. @chenba identified this a a permission
problem (See my #1856 which failed with

ERROR: (gcloud.auth.activate-service-account) Could not read json file /home/circleci/gcloud-service-key.json: Expecting value: line 2 column 1 (char 1)

After duplicating my PR into his #1861, all tests ran successfully. See his comment: #1856 (comment). Same error was reported with my other PR #1858

Cheers
-Fritz

[taddes]

I was that community contributor, so just a sidenote regarding:

"Your code must run and pass all the automated tests before you submit your PR"

If with "all the automated tests" you mean your circleCI, then this raises a chicken-egg problem, because only after submitting a PR those get triggered. I would suggest changing that to:

"Your code must run and pass all the automated tests before your PR is considered for review"

Furthermore, your circleCI is set up in a way that all tests are failing, because of some missing cloud key. It would be nice to have automatet tests run correctly for PRs. @chenba identified this a a permission problem (See my #1856 which failed with

ERROR: (gcloud.auth.activate-service-account) Could not read json file /home/circleci/gcloud-service-key.json: Expecting value: line 2 column 1 (char 1)

After duplicating my PR into his #1861, all tests ran successfully. See his comment: #1856 (comment). Same error was reported with my other PR #1858

Cheers -Fritz

Thanks @felfert . We won’t be changing permissions because of internal Mozilla security policies. I imagine you can understand why we wouldn’t want anyone to be able to run our CI pipelines to completion ad-hoc. I will double check if some other teams do it differently, but this has been the way for a while. I can clarify the language in the PR a little bit better it’s clear for community contributors too. This is also general purpose documentation for Mozilla engineers, so it paints with a bit of a broad brush. Thank you so much for your contributions and I’ll be sure to clarify this a bit.

[taddes]

I was that community contributor, so just a sidenote regarding:

"Your code must run and pass all the automated tests before you submit your PR"

If with "all the automated tests" you mean your circleCI, then this raises a chicken-egg problem, because only after submitting a PR those get triggered. I would suggest changing that to:

"Your code must run and pass all the automated tests before your PR is considered for review"

Furthermore, your circleCI is set up in a way that all tests are failing, because of some missing cloud key. It would be nice to have automatet tests run correctly for PRs. @chenba identified this a a permission problem (See my #1856 which failed with

ERROR: (gcloud.auth.activate-service-account) Could not read json file /home/circleci/gcloud-service-key.json: Expecting value: line 2 column 1 (char 1)

After duplicating my PR into his #1861, all tests ran successfully. See his comment: #1856 (comment). Same error was reported with my other PR #1858

Cheers -Fritz

However, I am a little bit confused about the GCP error. I’ll make sure we look into that. We don’t want it to be a bad experience for contributors

[felfert]

Thanks @felfert . We won’t be changing permissions because of internal Mozilla security policies. I imagine you can understand why we wouldn’t want anyone to be able to run our CI pipelines to completion ad-hoc. I will double check if some other teams do it differently, but this has been the way for a while. I can clarify the language in the PR a little bit better it’s clear for community contributors too. This is also general purpose documentation for Mozilla engineers, so it paints with a bit of a broad brush. Thank you so much for your contributions and I’ll be sure to clarify this a bit.

Well, if UTs are NOT running in a safe environment (e.g. docker containers that are limited to an isolated network) then I agree, but I also would say that your test setup is wrong security-wise. Unit tests should run in a sandbox where they could not affect anything outside of that environment.

Integration tests of course are different and those should not run at all when triggered by an outside entity.

So: If your UTs do NOT run in a sandbox, fix your test environment. (I don't know circleCI so unfortunately, I cannot help there).

Just my 2c
-Fritz

[taddes]

Thanks @felfert . We won’t be changing permissions because of internal Mozilla security policies. I imagine you can understand why we wouldn’t want anyone to be able to run our CI pipelines to completion ad-hoc. I will double check if some other teams do it differently, but this has been the way for a while. I can clarify the language in the PR a little bit better it’s clear for community contributors too. This is also general purpose documentation for Mozilla engineers, so it paints with a bit of a broad brush. Thank you so much for your contributions and I’ll be sure to clarify this a bit.

Well, if UTs are NOT running in a safe environment (e.g. docker containers that are limited to an isolated network) then I agree, but I also would say that your test setup is wrong security-wise. Unit tests should run in a sandbox where they could not affect anything outside of that environment.

Integration tests of course are different and those should not run at all when triggered by an outside entity.

So: If your UTs do NOT run in a sandbox, fix your test environment. (I don't know circleCI so unfortunately, I cannot help there).

Just my 2c -Fritz

Yeah, they run in a safe env. Exploring that Google auth issue you brought up.

[clouserw]

The google failure is to upload results/metrics of the tests -- not something we need to do for community contributions and we should fail gracefully. After that nothing should require any secrets.

That said, we're going to be moving to Github Actions at some point in the near future so we'll look at this more closely then. Thanks.