[#6136] refactor: move check for enable and disable within validate (CLI) #6163
Conversation
|
@justinmclean Is this what you envisioned when creating the issue? I was a bit confused by the wording. |
|
Had to modify to support the newly-refactored commands, but now should be ready for review @justinmclean |
clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java
Outdated
Show resolved
Hide resolved
clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java
Outdated
Show resolved
Hide resolved
|
@justinmclean Sorry for pinging you a lot but do things look good now? |
|
Sorry for the conflicting files again, let me sort this out for you. |
justinmclean
changed the title
|
No worries, it was a fairly quick fix on my end and shouldn't be a problem anymore given it seems like the refactoring that affects my files directly is done. |
| false) | ||
| .validate()); | ||
| String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); | ||
| Assertions.assertEquals(ErrorMessages.INVALID_ENABLE_DISABLE, errOutput); |
There was a problem hiding this comment.
I'm not sure why you have changed this test in this way. Can you explain the changes?
There was a problem hiding this comment.
My understanding is that I should detect if there's a RuntimeException when trying to enable and disable a catalog at the same time and then check if the error message is INVALID_ENABLE_DISABLE.
Is there an alternative to simplify this test or is am I misunderstanding the intent of these tests?
There was a problem hiding this comment.
So why change existing working tests that are needed? Did you mean to add a new test instead?
There was a problem hiding this comment.
Oh I see what you mean.
The reason why is because to check if both flags are true, I pass the disable flag's result into EnableCatalogCommand as a required parameter.
Is this the proper standard or not? Apologies for dragging this on for so long.
| GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", true, false) | ||
| .validate()); | ||
| String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); | ||
| Assertions.assertEquals(ErrorMessages.INVALID_ENABLE_DISABLE, errOutput); |
There was a problem hiding this comment.
Same here, I'm not sure these changes are required.
What changes were proposed in this pull request?
The check for enable and disable was within
GravitinoCommandLine.java, not inside thevalidate()logic as it should be.Why are the changes needed?
To keep consistency within the codebase and making sure the
validate()method for a command is being used effectivelyFix: #6136
Does this PR introduce any user-facing change?
No
How was this patch tested?
Updated tests and ran local test suite successfully.