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

fix: delete node_modules if frappe supports app_cache #1525

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

18alantom
Copy link
Member

Since part of the build executes in frappe, get-app cache requires at least v15.12.0 of frappe to function optimally i.e. delete node_modules and skip build. This PR checks frappe version mentioned in the app's pyproject.toml.

Uses version specifier format mentioned in: #1524

@18alantom 18alantom force-pushed the check-frappe-version branch 2 times, most recently from e318cb0 to 2e78e35 Compare January 31, 2024 12:22
@ankush
Copy link
Member

ankush commented Jan 31, 2024

@18alantom just few steps away from closing linked issue, might as well complete it 😬

@18alantom
Copy link
Member Author

@18alantom just few steps away from closing linked issue, might as well complete it 😬

@ankush I'm unsure about what all needs to be done w.r.t the linked issue:

  • How should dependency absence be handled, fail get-app or just show a warning?
  • Assuming frappe-dependencies are runtime, i.e. get-app can still run without the frappe-dependencies, doesn't make sense to fail it.
  • Any way to indicate strict requirement during get-app, (in case setup.py needs the dependency).

If we're looking to fail get-apps if mentioned dependency is not present, we should hold off until more thought is given. Press Deploy Candidate app ordering too would have to be updated to account for it.

@ankush
Copy link
Member

ankush commented Jan 31, 2024

Yes we should fail with non-zero exit code.

If ordering is a hard problem to solve we can optionally add a check command to do it after all apps are installed on bench?

Copy link

sonarcloud bot commented Feb 1, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@18alantom
Copy link
Member Author

Added dependency check, fails if folder not present. Also added a version check, doesn't fail but gives a warning if mismatch.

Regarding ordering:

  1. Ordering needs to be managed by whomever is installing apps using bench.
  2. If (all) apps have frappe-dependencies mentioned, invalid ordering is not possible (cause it will fail).
  3. If apps do not have frappe-dependencies mentioned, invalid ordering cannot be prevented (else get-app breaks).
  4. By point 2. and 3. ordering check in bench is redundant.

@18alantom 18alantom merged commit 436b3c5 into develop Feb 1, 2024
14 checks passed
Copy link

github-actions bot commented Feb 1, 2024

🎉 This PR is included in version 5.21.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

18alantom added a commit to frappe/press that referenced this pull request Feb 1, 2024
@ankush ankush deleted the check-frappe-version branch February 7, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants