Enhance setup.js for Improved Docker Compatibility #2447 #2615
Conversation
Bumps [sass](https://github.com/sass/dart-sass) from 1.80.6 to 1.80.7. - [Release notes](https://github.com/sass/dart-sass/releases) - [Changelog](https://github.com/sass/dart-sass/blob/main/CHANGELOG.md) - [Commits](sass/dart-sass@1.80.6...1.80.7) --- updated-dependencies: - dependency-name: sass dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…oesFoundation#2434) Bumps [eslint-plugin-import](https://github.com/import-js/eslint-plugin-import) from 2.30.0 to 2.31.0. - [Release notes](https://github.com/import-js/eslint-plugin-import/releases) - [Changelog](https://github.com/import-js/eslint-plugin-import/blob/main/CHANGELOG.md) - [Commits](import-js/eslint-plugin-import@v2.30.0...v2.31.0) --- updated-dependencies: - dependency-name: eslint-plugin-import dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…dation#2435) Bumps [@mui/x-charts](https://github.com/mui/mui-x/tree/HEAD/packages/x-charts) from 7.22.1 to 7.22.2. - [Release notes](https://github.com/mui/mui-x/releases) - [Changelog](https://github.com/mui/mui-x/blob/v7.22.2/CHANGELOG.md) - [Commits](https://github.com/mui/mui-x/commits/v7.22.2/packages/x-charts) --- updated-dependencies: - dependency-name: "@mui/x-charts" dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…dation#2436) Bumps [@types/react](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/react) from 18.3.3 to 18.3.12. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/react) --- updated-dependencies: - dependency-name: "@types/react" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
@palisadoes i had to make changes in Error: - Fixed File: - Previous File: - Reason for error: - In the schema file the name was defined as string but here the name was defined as object due to which there was error in data type. Can i make this change get merged as a part of my PR as many other contributors were facing the error. Due to which they were not able to make commit. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
src/setup/askForDocker/askForDocker.spec.ts (2)
1-7: Use consistent naming for test fileConsider aligning the test description in the
describeblock with the name of the function under test,askForDocker, instead ofaskForCustomPort, for clarity and consistency.-describe('askForCustomPort', () => { +describe('askForDocker', () => {
18-25: Robustness checkThis test case properly covers a custom port scenario. You might expand the test to include edge cases (e.g., invalid ports below 1024 or above 65535) if your production code eventually handles input validation.
src/setup/askForDocker/askForDocker.ts (2)
3-8: Consider returning a number instead of stringReturning a
numberwould ensure stronger type safety, especially since this value will likely be used in a numeric context (e.g., network port binding).
20-20: Validate the return type consistencyYou are returning
answers.dockerAppPortas a string. If you adopt numeric returns, convert this to a number. Make sure to keep the function usage consistent throughout the code.DOCUMENTATION.md (3)
28-28: Correct verb usageUse the base verb form after “should.”
- A local version of `docs.talawa.io` should automatically launched in your browser ... + A local version of `docs.talawa.io` should automatically launch in your browser ...🧰 Tools
🪛 LanguageTool
[grammar] ~28-~28: The modal verb ‘should’ requires the verb’s base form.
Context: ...fdocs.talawa.ioshould automatically launched in your browser at http://localhost:300...(MD_BASEFORM)
🪛 Markdownlint (0.37.0)
28-28: null
Bare URL used(MD034, no-bare-urls)
31-31: Fix typo in “brower”Typo found in the word "brower."
- Always monitor the local website in your brower ... + Always monitor the local website in your browser ...
36-36: Confirm preposition usageAdjust the sentence to clarify that new markdown files should go in the
talawa-docsrepository.- **_PLEASE_** do not add markdown files in this repository. Add them to `talawa-docs`! + **_PLEASE_** do not add markdown files to this repository. Add them to `talawa-docs`!🧰 Tools
🪛 LanguageTool
[uncategorized] ~36-~36: The preposition “to” seems more likely in this position.
Context: ... PLEASE do not add markdown files in this repository. Add them to `talawa-do...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
talawa-admin-docs/modules/screens_OrganizationDashboard_OrganizationDashboardMocks.md (1)
37-37: Enhance mock data realismThe mock data could be made more realistic by:
- Adding non-empty values for creator email fields
- Including some non-null image URLs to test image handling
- Using realistic image URLs instead of empty strings
Consider updating the mock data to include more realistic values:
creator`: \{ - `email`: `string` = ''; + `email`: `string` = '[email protected]'; `firstName`: `string` = ''; `lastName`: `string` = '' \};🧰 Tools
🪛 Markdownlint (0.37.0)
37-37: null
Bare URL used(MD034, no-bare-urls)
37-37: null
Bare URL used(MD034, no-bare-urls)
37-37: null
Bare URL used(MD034, no-bare-urls)
37-37: null
Bare URL used(MD034, no-bare-urls)
37-37: null
Bare URL used(MD034, no-bare-urls)
37-37: null
Bare URL used(MD034, no-bare-urls)
talawa-admin-docs/modules/screens_Users_UsersMocks.md (2)
27-27: Standardize email formats in mock dataThe email addresses in the mock data use inconsistent formats:
Standardize the format to improve consistency and maintainability.
Also applies to: 37-37
🧰 Tools
🪛 Markdownlint (0.37.0)
27-27: null
Bare URL used(MD034, no-bare-urls)
27-27: null
Bare URL used(MD034, no-bare-urls)
27-27: null
Bare URL used(MD034, no-bare-urls)
37-37: Consider consolidating duplicate mock data
MOCKS2appears to contain largely the same data asMOCKS. Consider:
- Consolidating common data into shared constants
- Only defining the differences in
MOCKS2Example approach:
const BASE_USER_MOCK = { _id: 'user1', firstName: 'John', // ... other common fields }; const MOCKS = [...]; const MOCKS2 = MOCKS.map(mock => ({ ...mock, // only override fields that need to be different }));🧰 Tools
🪛 Markdownlint (0.37.0)
37-37: null
Bare URL used(MD034, no-bare-urls)
37-37: null
Bare URL used(MD034, no-bare-urls)
37-37: null
Bare URL used(MD034, no-bare-urls)
CODE_STYLE.md (5)
57-58: Fix markdown formatting issuesPlease address the following formatting issues:
- Specify the language for the code block at line 58
- Use proper heading syntax instead of emphasis at line 64-65
Apply these changes:
-``` +```html-Correct ways ✅
+### Correct ways ✅Also applies to: 64-65 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 58-58: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `78-79`: **Improve documentation structure** Move the explanatory text outside the code block to maintain proper markdown structure and improve readability. ```diff -Using plain Bootstrap classes and attributes without leveraging the React-Bootstrap library should be refrained. While it may work for basic functionality, it doesn't fully integrate with React and may cause issues when dealing with more complex state management or component interactions. ```html <div class="dropdown">
250-254: Enhance SCSS compilation documentationThe SCSS compilation instructions should include context about when to use each command in the development workflow. Consider adding information about:
- When to use watch mode vs. one-time compilation
- Integration with the project's development scripts
- Best practices for source maps in different environments
119-121: Add Docker-specific development guidelinesSince this PR focuses on Docker compatibility improvements, consider adding a section about:
- Development workflow with Docker
- Best practices for running tests in Docker environment
- Guidelines for handling environment variables in Docker
Would you like me to help draft a Docker-specific development guidelines section?
128-140: Improve language consistency and conciseness
- Remove redundant "of the" phrases to make the text more concise
- Ensure consistent capitalization of "Sass" throughout the document
- Use proper verb agreement with singular proper nouns
Example improvements:
-This houses all of the static assets +This houses all static assets -Partial sass file +Partial Sass file🧰 Tools
🪛 LanguageTool
[style] ~128-~128: Consider removing “of” to be more concise
Context: ...tories ofsrcassets- This houses all of the static assets used in the project - `c...(ALL_OF_THE)
[style] ~130-~130: Consider removing “of” to be more concise
Context: ...d in the project -css- This houses all of the css files used in the project - `images...(ALL_OF_THE)
[style] ~131-~131: Consider removing “of” to be more concise
Context: ...in the project -images- This houses all of the images used in the project -scss- T...(ALL_OF_THE)
[style] ~132-~132: Consider removing “of” to be more concise
Context: ...d in the project -scss- This houses all of the scss files used in the project - `com...(ALL_OF_THE)
[grammar] ~136-~136: The singular proper name ‘Sass’ must be used with a third-person or a past tense verb.
Context: ...forms -_talawa.scss- Partial Sass file for Talawa -_utilities.scss- Part...(HE_VERB_AGR)
[grammar] ~137-~137: The singular proper name ‘Sass’ must be used with a third-person or a past tense verb.
Context: ...wa -_utilities.scss- Partial Sass file for utilities -_variables.scss- P...(HE_VERB_AGR)
[grammar] ~138-~138: The singular proper name ‘Sass’ must be used with a third-person or a past tense verb.
Context: ...es -_variables.scss- Partial Sass file for variables -app.scss- Main Sas...(HE_VERB_AGR)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
CODE_STYLE.md(9 hunks)DOCUMENTATION.md(2 hunks)package.json(1 hunks)src/setup/askForDocker/askForDocker.spec.ts(1 hunks)src/setup/askForDocker/askForDocker.ts(1 hunks)talawa-admin-docs/README.md(1 hunks)talawa-admin-docs/modules/components_OrgUpdate_OrgUpdate.md(1 hunks)talawa-admin-docs/modules/components_OrgUpdate_OrgUpdateMocks.md(1 hunks)talawa-admin-docs/modules/screens_OrganizationDashboard_OrganizationDashboardMocks.md(1 hunks)talawa-admin-docs/modules/screens_Users_UsersMocks.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- talawa-admin-docs/modules/components_OrgUpdate_OrgUpdate.md
🚧 Files skipped from review as they are similar to previous changes (1)
- talawa-admin-docs/README.md
🧰 Additional context used
🪛 Markdownlint (0.37.0)
CODE_STYLE.md
64-64: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
58-58: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
DOCUMENTATION.md
28-28: null
Bare URL used
(MD034, no-bare-urls)
talawa-admin-docs/modules/components_OrgUpdate_OrgUpdateMocks.md
17-17: null
Bare URL used
(MD034, no-bare-urls)
17-17: null
Bare URL used
(MD034, no-bare-urls)
17-17: null
Bare URL used
(MD034, no-bare-urls)
17-17: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
talawa-admin-docs/modules/screens_OrganizationDashboard_OrganizationDashboardMocks.md
17-17: null
Bare URL used
(MD034, no-bare-urls)
17-17: null
Bare URL used
(MD034, no-bare-urls)
17-17: null
Bare URL used
(MD034, no-bare-urls)
17-17: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
talawa-admin-docs/modules/screens_Users_UsersMocks.md
27-27: null
Bare URL used
(MD034, no-bare-urls)
27-27: null
Bare URL used
(MD034, no-bare-urls)
27-27: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
37-37: null
Bare URL used
(MD034, no-bare-urls)
🪛 LanguageTool
CODE_STYLE.md
[style] ~128-~128: Consider removing “of” to be more concise
Context: ...tories of src assets - This houses all of the static assets used in the project - `c...
(ALL_OF_THE)
[style] ~130-~130: Consider removing “of” to be more concise
Context: ...d in the project - css - This houses all of the css files used in the project - `images...
(ALL_OF_THE)
[style] ~131-~131: Consider removing “of” to be more concise
Context: ...in the project - images - This houses all of the images used in the project - scss - T...
(ALL_OF_THE)
[style] ~132-~132: Consider removing “of” to be more concise
Context: ...d in the project - scss - This houses all of the scss files used in the project - `com...
(ALL_OF_THE)
[grammar] ~136-~136: The singular proper name ‘Sass’ must be used with a third-person or a past tense verb.
Context: ...forms - _talawa.scss - Partial Sass file for Talawa - _utilities.scss - Part...
(HE_VERB_AGR)
[grammar] ~137-~137: The singular proper name ‘Sass’ must be used with a third-person or a past tense verb.
Context: ...wa - _utilities.scss - Partial Sass file for utilities - _variables.scss - P...
(HE_VERB_AGR)
[grammar] ~138-~138: The singular proper name ‘Sass’ must be used with a third-person or a past tense verb.
Context: ...es - _variables.scss - Partial Sass file for variables - app.scss - Main Sas...
(HE_VERB_AGR)
[uncategorized] ~166-~166: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...the following order: - React imports - Third party imports - Local imports If there is mo...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
DOCUMENTATION.md
[grammar] ~28-~28: The modal verb ‘should’ requires the verb’s base form.
Context: ...f docs.talawa.io should automatically launched in your browser at http://localhost:300...
(MD_BASEFORM)
[grammar] ~29-~29: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...ttp://localhost:3000/ 1. Add/modify the markdown documents to the docs/ directory of t...
(MARKDOWN_NNP)
[uncategorized] ~36-~36: The preposition “to” seems more likely in this position.
Context: ... PLEASE do not add markdown files in this repository. Add them to `talawa-do...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
talawa-admin-docs/modules/screens_OrganizationDashboard_OrganizationDashboardMocks.md
[duplication] ~10-~10: Possible typo: you repeated a word
Context: ...DashboardMocks.md#empty_mocks) - ERROR_MOCKS - [MOCKS](screens_OrganizationDashboard_Organiza...
(ENGLISH_WORD_REPEAT_RULE)
talawa-admin-docs/modules/screens_Users_UsersMocks.md
[duplication] ~9-~9: Possible typo: you repeated a word
Context: ...le of contents ### Variables - EMPTY_MOCKS - MOCKS - [...
(ENGLISH_WORD_REPEAT_RULE)
🪛 Biome (1.9.4)
src/setup/askForDocker/askForDocker.ts
[error] 12-12: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (3)
src/setup/askForDocker/askForDocker.spec.ts (1)
8-16: Tests coverage look good
The test effectively verifies the default Docker port scenario. Good job on mocking inquirer.prompt to isolate user input.
src/setup/askForDocker/askForDocker.ts (1)
10-12:
Use Number.isNaN for strict checking
Using the global isNaN can lead to unintended type coercions. Switch to Number.isNaN to ensure safer validations.
- if (isNaN(port) || port < 1024 || port > 65535) {
+ if (Number.isNaN(port) || port < 1024 || port > 65535) {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
package.json (1)
41-41: Upgrade confirmation
Good to see the inquirer dependency updated to ^12.3.0. Ensure that any new features or breaking changes in inquirer v12 are handled. Keep an eye on release notes for potential migration steps or improvements.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/app.module.css(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/style/app.module.css
[error] 21-21: Unexpected value or character.
Expected one of:
(parse)
[error] 21-21: Unexpected value or character.
Expected one of:
(parse)
🔇 Additional comments (1)
src/style/app.module.css (1)
21-22: Verify the purpose of loader-related changes.
These loader-related CSS variables seem unrelated to the PR's objective of enhancing Docker compatibility. Please clarify if these changes are necessary for the Docker setup improvements or if they should be moved to a separate PR.
🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: Unexpected value or character.
Expected one of:
(parse)
[error] 21-21: Unexpected value or character.
Expected one of:
(parse)
|
this failing test is due to another issue related to eslinting. Should i work and fix it myself and make it part of my PR? |
|
Also @palisadoes . All these failing tests are too many to cover in a single PR. It would exceed the scope of one PR.
|
|
|
This issue of failing tests has been encountered by many contributors. Even when they don't change any files, they face this problem. I suspect there might be an issue with the linter configuration or the CI pipeline. Should I let another contributor work on this issue, or should I work on it myself after opening it? @palisadoes |
|
I noticed that every PR has one test failing, so I think there is an issue with the CI pipeline. It seems that the workflow is not configured properly. |
|
What kind of change does this PR introduce?
feature
Issue Number:
Fixes #2446
Did you add tests for your changes?
No
Snapshots/Videos:
talawa-docker-setup-v1.mp4
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Chores
inquirer.