Skip to content

Conversation

@jeherve
Copy link
Member

@jeherve jeherve commented Jun 3, 2019

This replaces #12384

Changes proposed in this Pull Request:

  • This PR introduces a set of standards (currently a copy of existing Jetpack standards) that could be used in other projects by just adding the package.
  • Those standards are currently divided into 2 main categories:
    • dotfiles that are useful for development, but do not necessarily need to be checked in the repo, and can be shared with other repos.
    • Repo configuration files, that are used in combination with a GitHub webhook to enforce certain defaults (set of labels, branch protection, merge settings) on a repo when the files are present.

In future iterations, this set of standards will include more standards:

  • GitHub issue / PR templates.
  • Contributing docs.
  • Release scripts

As is, this PR brings in changes from what is currently another repo. Once this is merged, the other repo will become a mirror of the package living in the Jetpack repo.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Part of the DNA project.

Note: This PR currently includes the fix from #12546. I can rebase later on once the other PR is merged.

TBD: I think I may end up adding the 2 config files in .github/ to .gitignore, like the other dotfiles. We don't need them in the Jetpack repo since the repo was already configured.

Testing instructions:

  • Check out this branch.
  • Run yarn build
  • Check that the dotfiles that were removed in this PR are all added back to your working local copy.
  • Check that the .github/ directory now includes 2 new files, labels.json and settings.json.

Proposed changelog entry for your changes:

  • Jetpack Standards: introduce new development package bundling useful tools to create and maintain a Jetpack project.

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Focus] Jetpack DNA labels Jun 3, 2019
@jeherve jeherve added this to the 7.5 milestone Jun 3, 2019
@jeherve jeherve requested a review from a team June 3, 2019 22:45
@jeherve jeherve self-assigned this Jun 3, 2019
@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Jun 3, 2019
@simison
Copy link
Member

simison commented Jun 4, 2019

Should a legacy think like .jshintrc be just left out? Note that in Jetpack repo it's limited to some specific folders with .jshintignore + list in Gulp config and omitting that file will cause both jshint and eslint run on all files, which would conflict.

@simison
Copy link
Member

simison commented Jun 4, 2019

Since Travis is picking Node.js version for the container based on .nvmrc file, it has to be there before anything runs. Otherwise, it picks some old Node.js version.

Alternatively, it's possible to hardcode Node.js version in Travis config but that's kinda defying the purpose of that file. :-)

Note also that supportPolicy Renovate config is keeping .nvmrc file up to date, not sure if you can tell Renovate to look elsewhere than the root folder.

@jeherve jeherve added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Jun 4, 2019
@jeherve
Copy link
Member Author

jeherve commented Jun 4, 2019

Should a legacy think like .jshintrc be just left out?

@simison What do you think about keeping that file around, but with a comment to let folks know not to edit it directly?

Since Travis is picking Node.js version for the container based on .nvmrc file, it has to be there before anything runs. Otherwise, it picks some old Node.js version.

Wouldn't it run the Node.js version specified in the engines section of package.json? In fact, do we need .nvmrc when we specify a node version in package.json? (This may be a stupid question)

@jeherve jeherve force-pushed the dna/standards branch 2 times, most recently from 883ed77 to a14edca Compare June 4, 2019 15:58
jeherve added a commit that referenced this pull request Jun 4, 2019
jeherve added a commit that referenced this pull request Jun 4, 2019
@jetpackbot
Copy link
Collaborator

jetpackbot commented Jun 4, 2019

Warnings
⚠️ The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against e7bd6e8

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Jun 4, 2019
@jeherve
Copy link
Member Author

jeherve commented Jun 4, 2019

I added both files back to avoid test failures for now. We'll need to find a way to make sure contributions to those source-controlled files (from external contributors or from Renovate) make it to the package files somehow.

@simison
Copy link
Member

simison commented Jun 5, 2019

Should a legacy think like .jshintrc be just left out?

What do you think about keeping that file around,

I think IDEs will pick it up since the file is there if you have jshint configured in the editor. It's also confusing to have a config file in the project that's unused.

but with a comment to let folks know not to edit it directly?

Can you elaborate on what you mean? :-)

Since Travis is picking Node.js version for the container based on .nvmrc file, it has to be there before anything runs. Otherwise, it picks some old Node.js version.

Wouldn't it run the Node.js version specified in the engines section of package.json? In fact, do we need .nvmrc when we specify a node version in package.json? (This may be a stupid question)

There are no such questions! :-) No worries.

Seems like only Travis config and nvmrc file are supported by Travis: https://docs.travis-ci.com/user/languages/javascript-with-nodejs/#specifying-nodejs-versions

NVM users will need the file there so that nvm use or automatic Node.js version switching will work.

@simison
Copy link
Member

simison commented Jun 5, 2019

Just wanted to note that I still think there are better, more developer friendly and more established mechanisms to share configs on tool-by-tool basis rather than in en-masse like this. Convo in the previous PR: #12384

Curious to see how this will play out tho!

@simison
Copy link
Member

simison commented Jun 7, 2019

Some chatter p1559752054055100-slack-jetpack-dna

@jeherve jeherve added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Jun 7, 2019
@dereksmart dereksmart changed the base branch from feature/jetpack-packages to feature/jetpack-packages-2 June 7, 2019 21:42
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D29450-code before merging this PR. Thank you!

@matticbot
Copy link
Contributor

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "record_user_event" function.

cc: @pesieminski

@matticbot
Copy link
Contributor

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "record_user_event" function.

cc: @pesieminski

@matticbot
Copy link
Contributor

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "record_user_event" function.

cc: @pesieminski

@dereksmart dereksmart changed the base branch from feature/jetpack-packages-2 to master June 17, 2019 14:16
@jeherve jeherve removed this from the 7.5 milestone Jun 17, 2019
@dereksmart dereksmart added [Status] Needs Tracks Review Added/removed/modified a tracks event. and removed [Status] Needs GDPR Review labels Nov 6, 2019
@kraftbj
Copy link
Contributor

kraftbj commented Jun 1, 2020

We can include #15693 as a dependency of this is we want to.

@kraftbj kraftbj removed the request for review from a team June 1, 2020 19:49
@kraftbj kraftbj added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress [Status] Needs Tracks Review Added/removed/modified a tracks event. labels Jan 30, 2021
@kraftbj
Copy link
Contributor

kraftbj commented Jan 30, 2021

I still like the idea of this, but I paired it down to the settings.json that is unique. It still require a webhook, though.

@anomiex
Copy link
Contributor

anomiex commented Feb 18, 2021

Looks like the PR description could use an update. Also, I was unable to locate documentation about this settings.json file anywhere, unless it's this bot that would have to be added to the new repo.

@anomiex anomiex added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Feb 18, 2021
@github-actions github-actions bot added the Actions GitHub actions used to automate some of the work around releases and repository management label Feb 18, 2021
@jeherve
Copy link
Member Author

jeherve commented Feb 25, 2021

Yeah, I don't think this is really worth it anymore. There is very little left into it, and it does require some manual changes in the settings of a new repo to add the webhook anyway. At this point, we might as well do the changes manually.

I think this won't be really necessary once we automate repo creation. I'll close this PR.

@jeherve jeherve closed this Feb 25, 2021
@jeherve jeherve deleted the dna/standards branch February 25, 2021 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Actions GitHub actions used to automate some of the work around releases and repository management [Focus] Jetpack DNA [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants