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

Feature/centralize dependencies in root #2746

Conversation

SuperITMan
Copy link
Member

@SuperITMan SuperITMan commented May 24, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[X] Build related changes
[X] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Dependencies are duplicated in root folder and in stark packages folders.
Moreover, some dependencies are not in sync between packages.

Due to NodeJS behavior, dependencies installed in root are available in showcase and in starter.
This can have side effects.

TSLint is not up to date in stark-build dependencies.

Issue Number: #2695

What is the new behavior?

All the dependencies are installed only in root folder. When a PR is introduced, the build checks if devDependencies versions in root "package.json" are in sync with dependencies version in stark packages.
If this is not the case, the build will fail. If the PR has been created by Dependabot, another workflow will be triggered and will update dependencies version in stark packages. After that, the workflow, thanks to the newcomer @nbb-bot 🤖, will push new commits to the Dependabot branch and will add a comment to the PR to mention that new commits have been added to the branch. This will trigger the "build" workflow that should work this time 👍

To solve the potential issue due to root dependencies available in showcase and starter, I refactored the build workflow to remove root dependencies before installing and running tests in showcase and starter.
Moreover, for Dependabot PRs, I adapted the build to remove the "package-lock.json" file in showcase to ensure it installs the right dependencies after updating stark-* dependencies.

TSLint is updated to v6.1 in stark-build dependencies.

Does this PR introduce a breaking change?

[X] Yes
[ ] No

Other information

    BREAKING CHANGE:
    Due to TSLint update, it is required to adapt the usage in "tslint.json" file at the root of the project:

    ```txt
    // Before
    {
      "extends": [
        // ...
        "@nationalbankbelgium/code-style/tslint/5.20.x",
        // ...
      ]
      // ...
    }

    // After
    {
      "extends": [
        // ...
        "@nationalbankbelgium/code-style/tslint/6.1.x",
        // ...
      ]
      // ...
    }
@SuperITMan SuperITMan added comp: build-main Main build comp: stark-all To apply for all Stark modules breaking change type: feature dependencies Pull requests that update a dependency file comp: ci & tooling CI, GitHub Actions, ... labels May 24, 2021
@SuperITMan SuperITMan added this to the 11.0.0-beta.0 milestone May 24, 2021
@sonarcloud
Copy link

sonarcloud bot commented May 24, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.154% when pulling 4c7696e on SuperITMan:feature/centralize-dependencies-in-root into 0f05c34 on NationalBankBelgium:master.

@SuperITMan
Copy link
Member Author

A demo of Dependabot PR can be found on my fork: SuperITMan#1151

Copy link
Contributor

@nicanac nicanac left a comment

Choose a reason for hiding this comment

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

Seems good to me,
It's probably going to solve my issue with the dep.
👍

@SuperITMan SuperITMan merged commit 9c0f640 into NationalBankBelgium:master May 25, 2021
SuperITMan added a commit to SuperITMan/stark that referenced this pull request Sep 2, 2021
SuperITMan added a commit to SuperITMan/stark that referenced this pull request Sep 2, 2021
SuperITMan added a commit to SuperITMan/stark that referenced this pull request Sep 2, 2021
SuperITMan added a commit to SuperITMan/stark that referenced this pull request Sep 2, 2021
@SuperITMan SuperITMan deleted the feature/centralize-dependencies-in-root branch February 5, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change comp: build-main Main build comp: ci & tooling CI, GitHub Actions, ... comp: stark-all To apply for all Stark modules dependencies Pull requests that update a dependency file type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants