carvel-dev / carvel-dev/kapp

Cleanup: preflight checks

Open
#945 3 comments 0 reactions 1 assignee Claimed by @everettraven View on GitHub
carvel accepted cleanup
Dominant language
Go
Stars
1.1k
Forks
134
PR merge metrics
No merged PRs in 30d

Description

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](https://github.com/carvel-dev/kapp/blob/develop/pkg/kapp/preflight/registry_test.go))
- [ ] Remove the usages of `RemoveClusterResource` in tests that are no longer required ([example](https://github.com/carvel-dev/kapp/blob/0e94ff15321f67ba737df346bd29e82cd62ee057/test/e2e/preflight_permission_validation_escalation_test.go#L84))
- [ ] 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](https://github.com/carvel-dev/kapp/blob/0e94ff15321f67ba737df346bd29e82cd62ee057/cmd/kapp/kapp.go#L45). We should aim to keep the error format consistent ([example](https://github.com/carvel-dev/kapp/blob/0e94ff15321f67ba737df346bd29e82cd62ee057/test/e2e/apply_wait_error_test.go#L208) 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](https://github.com/carvel-dev/kapp/blob/0e94ff15321f67ba737df346bd29e82cd62ee057/pkg/kapp/clusterapply/waiting_changes.go#L143) to ensure that each individual error gets it's own line making the overall error more clear. ([example](https://github.com/carvel-dev/kapp/blob/0e94ff15321f67ba737df346bd29e82cd62ee057/test/e2e/formatted_error_test.go#L53) 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](https://github.com/carvel-dev/kapp/pull/950#discussion_r1588263642))

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.