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

Cleanup: preflight checks #945

Open
6 tasks
praveenrewar opened this issue Apr 29, 2024 · 3 comments
Open
6 tasks

Cleanup: preflight checks #945

praveenrewar opened this issue Apr 29, 2024 · 3 comments
Assignees
Labels
carvel accepted This issue should be considered for future work and that the triage process has been completed cleanup cleaning up code, process, or technical debt

Comments

@praveenrewar
Copy link
Member

praveenrewar commented Apr 29, 2024

Recently we have introduced the preflight flag in kapp and also added a couple of checks. This issue tracks a few of the items that we didn't cover during the reviews and it would be good to do some clean up.

  • Package name for test files should be *_test (registry_test.go)
  • Remove the usages of RemoveClusterResource in tests that are no longer required (example)
  • In Carvel, we capitalise first character of error messages along with colons, this is used in the ui library to help with printing multiline error messages. We should aim to keep the error format consistent (example of how the error looks like with indentation)
  • In some scenarios, there might be more than one errors that we want to report, we use semi structured error to ensure that each individual error gets it's own line making the overall error more clear. (example of how such an error message looks like)
  • We seem to be adding a lot of e2e tests for preflight checks (which is good). Currently all these tests are in the test/e2e directory and most of them have a dedicated file (which is also good as # of lines per file are less). Should we move them to a dedicated directory like test/e2e/preflights so that the other e2e tests are not overshadowed by preflight checks?
  • Fix handled() function for some of the validators (details)
@praveenrewar praveenrewar added carvel accepted This issue should be considered for future work and that the triage process has been completed cleanup cleaning up code, process, or technical debt labels Apr 29, 2024
@praveenrewar
Copy link
Member Author

@everettraven Let me know if this looks good.

@everettraven
Copy link
Contributor

Looks great, thanks for writing this up!

@rashmigottipati
Copy link
Contributor

As a part of this issue, we also need to address this particular review comment for all the instances where we return handled() so that empty values are not returned for the diff objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel accepted This issue should be considered for future work and that the triage process has been completed cleanup cleaning up code, process, or technical debt
Projects
Status: No status
Development

No branches or pull requests

3 participants