Add check for image name length to not exceed 255
#2555
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Devashishbasu
requested review from
manusa,
rohanKanojia and
sunix
as code owners
Eclipse JKube CI ReportStarted new GH workflow run for #2555 (2024-01-29T12:25:33Z) ⚙️ JKube E2E Tests (7695862270)
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2555 +/- ##
=============================================
+ Coverage 59.36% 69.83% +10.47%
- Complexity 4586 4902 +316
=============================================
Files 500 474 -26
Lines 21211 19175 -2036
Branches 2830 2481 -349
=============================================
+ Hits 12591 13391 +800
+ Misses 7370 4565 -2805
+ Partials 1250 1219 -31 ☔ View full report in Codecov by Sentry. |
do i need to also uncomment the line 98 ? |
|
@Devashishbasu : Yes, but in the PR where you fix that issue. |
uncommented test
done |
when i uncommented the build got failed ...how can i fix this?? |
|
@Devashishbasu : You would need to debug why your fix is not aligning with what's defined in the test. Expected behavior is to throw exception on name length greater than 255 |
updating changes
added check for repository
|
@rohanKanojia @manusa still stuck with this test case "repo@sha256:ffffffffffffffffffffffffffffffffff" can you please review where i am making mistake? |
|
@Devashishbasu : Are you sure you should be uncommenting that test case in this PR? In #2555 (comment) I meant uncommenting it in #2557 not in this pull request. |
thankyou so much for clearing my doubt |
| @@ -288,6 +294,11 @@ private void doValidate() { | |||
| checks[i], value, checkPattern.pattern())); | |||
| } | |||
| } | |||
|
|
|||
| if (repository.length() > REPO_NAME_MAX_LENGTH) { | |||
| errors.add(String.format("Repository name must not be more than %d characters", REPO_NAME_MAX_LENGTH)); | |||
There was a problem hiding this comment.
Could you please add one more test in ImageNameTest for verifying exception with this exact message is thrown when too long image name is provided?
There was a problem hiding this comment.
Could you please add one more test in ImageNameTest for verifying exception with this exact message is thrown when too long image name is provided?
Ya sure
added test case
| private String generateTooLongImageName() { | ||
| StringBuilder tooLongName = new StringBuilder(); | ||
| int maxLength = 255 + 1; // exceeding the maximum length | ||
| for (int i = 0; i < maxLength; i++) { |
There was a problem hiding this comment.
You can replace this by a prebuilt method from Apache's StringUtils repeat
|
|
||
| // New test for too long repository name | ||
| String tooLongName = generateTooLongImageName(); | ||
| assertThatIllegalArgumentException() |
There was a problem hiding this comment.
Sorry, I meant to add a new test; not modify existing test. Could you please move this code block into a separate test?
@Test
void shouldThrowExceptionOnTooLongImageName() {
}There was a problem hiding this comment.
Sorry, I meant to add a new test; not modify existing test. Could you please move this code block into a separate test?
@Test void shouldThrowExceptionOnTooLongImageName() { }
Hii @rohanKanojia did all the changes you told me do, but getting a build failure
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:38 min
[INFO] Finished at: 2024-01-22T14:44:48Z
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.1.2:test (default-test) on project jkube-kit-remote-dev: There are test failures.
[ERROR]
[ERROR] Please refer to /home/jenkins/agent/workspace/Build_Java8_PR-2555/jkube-kit/remote-dev/target/surefire-reports for the individual test results.
[ERROR] Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
[ERROR] -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.1.2:test (default-test) on project jkube-kit-remote-dev: There are test failures.
is it due to my changes? or can you suggest how can i fix this?
There was a problem hiding this comment.
Sorry, I meant to add a new test; not modify existing test. Could you please move this code block into a separate test?
@Test void shouldThrowExceptionOnTooLongImageName() { }Hii @rohanKanojia did all the changes you told me do, but getting a build failure
[INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ [INFO] Total time: 03:38 min [INFO] Finished at: 2024-01-22T14:44:48Z [INFO] ------------------------------------------------------------------------ [ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.1.2:test (default-test) on project jkube-kit-remote-dev: There are test failures. [ERROR] [ERROR] Please refer to /home/jenkins/agent/workspace/Build_Java8_PR-2555/jkube-kit/remote-dev/target/surefire-reports for the individual test results. [ERROR] Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream. [ERROR] -> [Help 1] org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.1.2:test (default-test) on project jkube-kit-remote-dev: There are test failures.
is it due to my changes? or can you suggest how can i fix this?
test added by me is passing
There was a problem hiding this comment.
@Devashishbasu : This failure seems unrelated to your changes. Probably it's a flaky test that needs to be fixed. Could you please try pushing again and see if you see the issue again?
There was a problem hiding this comment.
@Devashishbasu : This failure seems unrelated to your changes. Probably it's a flaky test that needs to be fixed. Could you please try pushing again and see if you see the issue again?
Hi @rohanKanojia i've pushed the code again and seeing the same issue again...it is the same issue which you have opened #2576
There was a problem hiding this comment.
@Devashishbasu : This failure seems unrelated to your changes. Probably it's a flaky test that needs to be fixed. Could you please try pushing again and see if you see the issue again?
Hii @rohanKanojia ...still the same issue persists
|
|
Hii @manusa can you please review it? |
|
@Devashishbasu : Your branch does not contain the commit where I disabled the failing test on jdk8. You need to rebase this branch with the latest updates in master. |
|
@Devashishbasu : Please squash your commits into one commit. |
rohanKanojia
changed the title
255
rohanKanojia
changed the title
255255
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #2542