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

Enhancements to care config #8470 #8726

Conversation

Sulochan-khadka
Copy link
Contributor

Proposed Changes

@ohcnetwork/care-fe-code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

@Sulochan-khadka Sulochan-khadka requested a review from a team as a code owner October 6, 2024 08:20
Copy link

netlify bot commented Oct 6, 2024

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit 1bd323b
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/6703ebe04f04600008b64b47
😎 Deploy Preview https://deploy-preview-8726--care-ohc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Sulochan-khadka
Copy link
Contributor Author

New to this kind of Issues, so kindly let me know if there's something needed, or if there are mistakes.

@khavinshankar
Copy link
Member

@Sulochan-khadka can you please fix the failing cypress tests

@khavinshankar khavinshankar added the cypress failed pull request with cypress test failure label Oct 7, 2024
@Sulochan-khadka
Copy link
Contributor Author

@Sulochan-khadka can you please fix the failing cypress tests

i have fixed all cypress tests but dont know why one is failing. May you help!

@bodhish
Copy link
Member

bodhish commented Oct 7, 2024

Not sure if this is the right approach, i was thinking that we would add some sort of json validator that would be run before the build step. 🤔

@rithviknishad please review the PR

@bodhish bodhish requested a review from rithviknishad October 7, 2024 22:59
Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

This does not validate the env's during the build stage. That is the main requirement mentioned in the issue.

image

We could add a script and call that script (that imports care.config.ts) before the vite build. If validation fails, exit with a non-zero status code so that vite build does not start (as vite build should happen only after the validation script runs).

@rithviknishad rithviknishad added changes required invalid This doesn't seem right labels Oct 8, 2024
@Sulochan-khadka
Copy link
Contributor Author

This does not validate the env's during the build stage. That is the main requirement mentioned in the issue.

image We could add a script and call that script (that imports `care.config.ts`) before the vite build. If validation fails, exit with a non-zero status code so that vite build does not start (as vite build should happen only after the validation script runs).

Ok so the required task is to create some script like "validate-env.ts" that imports care.config.ts and checks if the necessary environment variables are defined or valid.
And then run through this script before vite build and only proceeds to vite build if the script approves.
Am i on the right track now? If yes, should i close this PR and raise a new one?

@rithviknishad
Copy link
Member

Yes; you are right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes required cypress failed pull request with cypress test failure invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancements to care config
4 participants