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

Setup script for .env configuration file #1350

Closed
wants to merge 25 commits into from

Conversation

disha1202
Copy link
Contributor

@disha1202 disha1202 commented Jan 2, 2024

What kind of change does this PR introduce?

Issue Number:

Fixes #1233

Did you add tests for your changes?

No

Snapshots/Videos:

Screen.Recording.2024-01-02.at.12.18.53.PM.mov

If relevant, did you update the documentation?

Summary

Does this PR introduce a breaking change?

Other information

Have you read the contributing guide?

Copy link

github-actions bot commented Jan 2, 2024

Our Pull Request Approval Process

We have these basic policies to make the approval process smoother for our volunteer team.

Testing Your Code

Please make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:

  1. The overall code coverage drops below the target threshold of the repository
  2. Any file in the pull request has code coverage levels below the repository threshold
  3. Merge conflicts

The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing.

Reviewers

When your PR has been assigned reviewers contact them to get your code reviewed and approved via:

  1. comments in this PR or
  2. our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

  1. arbitrators of future discussions with other contributors about the validity of your changes
  2. point of contact for evaluating the validity of your work
  3. person who verifies matching issues by others that should be closed.
  4. person who gives general guidance in fixing your tests

CONTRIBUTING.md

Read our CONTRIBUTING.md file. Most importantly:

  1. PRs with issues not assigned to you will be closed by the reviewer
  2. Fix the first comment in the PR so that each issue listed automatically closes

Other

  1. 🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise.
  2. Read the CONTRIBUTING.md file make

Copy link

codecov bot commented Jan 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (45920a7) 97.39% compared to head (49069c3) 96.99%.

❗ Current head 49069c3 differs from pull request most recent head cb73e16. Consider uploading reports for the commit cb73e16 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1350      +/-   ##
===========================================
- Coverage    97.39%   96.99%   -0.40%     
===========================================
  Files          143      137       -6     
  Lines         3640     3426     -214     
  Branches      1073      963     -110     
===========================================
- Hits          3545     3323     -222     
- Misses          90       98       +8     
  Partials         5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SiddheshKukade
Copy link
Member

Will check it thoroughly and confirm later.

@palisadoes
Copy link
Contributor

@SiddheshKukade Please remember your review. It's been some time

@SiddheshKukade
Copy link
Member

Looks good. I tried a few combinations

SiddheshKukade
SiddheshKukade previously approved these changes Jan 6, 2024
Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

@disha1202

You have not updated INSTALLATION.md to make people know that the setup script exists
Use the one in the API repo as a guide

@disha1202
Copy link
Contributor Author

@palisadoes

I have updated the installation guide.
Please take a look.

@disha1202 disha1202 requested a review from palisadoes January 8, 2024 07:07
Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

@disha1202

  1. The script does not check to see whether the API is actually running
  2. The script assumes any character that is not N is yes. This is not valid
  3. If you make any edits to the .env file in the script then any pre-existing comments already existing in the .env file are removed
  4. If you make any edits to the .env file in the script and then hit CTRL-C then any pre-existing comments already existing in the .env file are removed
  5. The script must assume a default of Y, if a pre-existing configuration parameter exists
  6. If any value in the .env file is commented out but the rest are valid, the script fails telling you Please copy the contents of .env.example to .env file This is not valid. The script should automatically insert missing values

Please test the script more thoroughly

Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

Please fix the failing test

…igurations exists, default Y is assumed, automatically insert missing values
@disha1202
Copy link
Contributor Author

disha1202 commented Jan 11, 2024

Hi @palisadoes

I have made all the requested changes and also tested the script.

The tests are failing because of type errors in src/screens/OrgPost/OrgPost.tsx
which is not related to the changes in this PR.

Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

@disha1202

  • I can't test. It hangs when testing the API URL when the URL is valid.

@disha1202
Copy link
Contributor Author

disha1202 commented Jan 14, 2024

@disha1202

  • I can't test. It hangs when testing the API URL when the URL is valid.

It takes a little bit of time because we are making an API call to test whether the server is running or not.

Screen.Recording.2024-01-14.at.7.21.57.PM-2.mov

@disha1202
Copy link
Contributor Author

@palisadoes
Fixed the failing tests which were related to this PR.

palisadoes
palisadoes previously approved these changes Jan 18, 2024
@palisadoes
Copy link
Contributor

The tests are still failing. Please merge your code with the latest upstream

@disha1202
Copy link
Contributor Author

Updated the PR with the latest upstream.

The tests are still failing.

@palisadoes
Copy link
Contributor

Tests are still failing

@palisadoes
Copy link
Contributor

@disha1202

  1. Merge your code with the latest upstream that may fix the failing Test Application tests.
  2. The introspection one is being fixed elsewhere.

@disha1202
Copy link
Contributor Author

@disha1202

  1. Merge your code with the latest upstream that may fix the failing Test Application tests.
  2. The introspection one is being fixed elsewhere.

@palisadoes

I have updated the PR with the latest changes but the tests are still failing.

@palisadoes
Copy link
Contributor

This file shouldn't be a part of this PR. That may be the cause

  • src/screens/OrgPost/OrgPost.tsx

@disha1202
Copy link
Contributor Author

This file shouldn't be a part of this PR. That may be the cause

  • src/screens/OrgPost/OrgPost.tsx

I updated src/screens/OrgPost/OrgPost.tsx to fix the type errors.

Screenshot 2024-01-19 at 9 40 34 PM

@palisadoes
Copy link
Contributor

That file isn't required in this PR. It must not need included.

@palisadoes
Copy link
Contributor

@disha1202

If this file is no longer in the PR, why is it being linted in the tests? Please make a minor commit to see whether it triggers the correct action

  • src/screens/OrgPost/OrgPost.tsx

@palisadoes
Copy link
Contributor

palisadoes commented Jan 19, 2024

@disha1202

  1. We need this completed soon.
  2. We have other PRs depending on updating the setup script. Like this one:

@disha1202
Copy link
Contributor Author

disha1202 commented Jan 20, 2024

@disha1202

If this file is no longer in the PR, why is it being linted in the tests? Please make a minor commit to see whether it triggers the correct action

  • src/screens/OrgPost/OrgPost.tsx

The tests are still failing for src/screens/OrgPost/OrgPost.tsx.

Should I try creating a new PR?

@palisadoes
Copy link
Contributor

Yes. Please create a new one.

Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants