-
Notifications
You must be signed in to change notification settings - Fork 463
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
feat(jkube-kit/generator): oc:build or k8s:build should warn the user that he may have forgotten to run a build or package #2244
Conversation
Eclipse JKube CI ReportStarted new GH workflow run for #2244 (2023-11-22T08:54:47Z) ⚙️ JKube E2E Tests (6954860076)
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2244 +/- ##
============================================
+ Coverage 59.36% 61.38% +2.01%
- Complexity 4586 4872 +286
============================================
Files 500 522 +22
Lines 21211 21598 +387
Branches 2830 2875 +45
============================================
+ Hits 12591 13257 +666
+ Misses 7370 7106 -264
+ Partials 1250 1235 -15 ☔ View full report in Codecov by Sentry. |
...e-kit/generator/webapp/src/main/java/org/eclipse/jkube/generator/webapp/WebAppGenerator.java
Outdated
Show resolved
Hide resolved
...e-kit/generator/webapp/src/main/java/org/eclipse/jkube/generator/webapp/WebAppGenerator.java
Outdated
Show resolved
Hide resolved
Please make sure you also implement tests that validate your fix before creating PR. |
because I'm using the Eclipse code formatter provided with this project. So, every time I change a file which had incorrect code format, the code that has nothing to do with my logic will also change. So, I make a PR for the code format, and then continue writing my code by checking out from the code format branch (hence the code format commit will be there in my fix too). Once you merge the code format PR into master, reviewing my code will be easier because of this redundant commit. |
@rohanKanojia there's a Sonar issue here related to my use of Md5 hash. It should be ignored as we are not using for encryption but instead simply to check the similarity of two files. |
@rohanKanojia I have added the test cases. |
jkube-kit/generator/webapp/src/main/java/org/eclipse/jkube/generator/webapp/ArtifactUtil.java
Outdated
Show resolved
Hide resolved
jkube-kit/generator/webapp/src/main/java/org/eclipse/jkube/generator/webapp/ArtifactUtil.java
Outdated
Show resolved
Hide resolved
jkube-kit/generator/webapp/src/main/java/org/eclipse/jkube/generator/webapp/ArtifactUtil.java
Outdated
Show resolved
Hide resolved
...-kit/generator/webapp/src/test/java/org/eclipse/jkube/generator/webapp/ArtifactUtilTest.java
Outdated
Show resolved
Hide resolved
...-kit/generator/webapp/src/test/java/org/eclipse/jkube/generator/webapp/ArtifactUtilTest.java
Outdated
Show resolved
Hide resolved
We have migrated some workflows to Jenkins, could you please rebase your PR against latest master? |
jkube-kit/generator/webapp/src/main/java/org/eclipse/jkube/generator/webapp/ArtifactUtil.java
Outdated
Show resolved
Hide resolved
@rohanKanojia @sunix for the Sonar security hotspot issue in ArtifactUtil, I have added a SuppressWarnings annotation, but it's not working. I think instead of ignoring it in the code, we should properly add it as an exclusion rule in sonar config file. I think the project maintainers should do it. But if you say, I can do it myself. |
jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/ArtifactUtil.java
Outdated
Show resolved
Hide resolved
jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/ArtifactUtil.java
Outdated
Show resolved
Hide resolved
At the moment, it is just doing it for webapp projects. It is fine, for a first step, but it needs to be clear in the PR description and in the issue description that you are doing it in several steps. |
@sunix License check fails for ArtifactUtil and ArtifactUtilTest files, but I already added the license there. Could you please check ? |
@ShivangMishra You need to fix
|
|
||
} | ||
|
||
public static boolean isArtifactSameAsPrevious(File sourceFile, Path checksumPath, PrefixedLogger log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I too noticed that, but couldn't think of a better name.
jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/ArtifactUtil.java
Outdated
Show resolved
Hide resolved
I'm testing this but seeing strange behavior. I'm seeing warning getting printed always on second run of
|
long currentLastModifiedTime = finalOutputArtifact.lastModified(); | ||
long previousLastModifiedTime = retrievePreviousArtifactLastModifiedTime(lastModifiedTimeSavePath); | ||
|
||
boolean isCurrentFinalArtifactSameAsPrevious = currentLastModifiedTime == previousLastModifiedTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would make plugin print warning always after first k8s:build
run. Do we really want this behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would make plugin print warning always after first
k8s:build
run. Do we really want this behavior?
but we don't get any warning after first k8s:build. Warning is issued only if curentLastModifiedTime is equal to previousLastModifiedTime. On the first run, this will be false.
yes we get a warning on running k8s:build again without running mvn package. The user may or may not have made any changes. In either case, if they don't run mvn package (intentionally or unintentionally) before k8s:build, we warn them. |
6589781
to
4b2db5e
Compare
I'm not convinced with this. With this approach we're introducing some false positive warnings. |
...e-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java
Outdated
Show resolved
Hide resolved
jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/ArtifactUtil.java
Outdated
Show resolved
Hide resolved
...e-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java
Outdated
Show resolved
Hide resolved
...e-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java
Outdated
Show resolved
Hide resolved
...e-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java
Outdated
Show resolved
Hide resolved
...e-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java
Outdated
Show resolved
Hide resolved
...e-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java
Outdated
Show resolved
Hide resolved
de8df0f
to
a0a1d0b
Compare
Kudos, SonarCloud Quality Gate passed!
|
37d465f
to
8f983f0
Compare
… that he may have forgotten to run a build or package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rolled-back checks in many of the generators where this
logic wasn't applied correctly and some false positives
would be logged in case of different packaging formats.
In the future, we might want to consider adding the warning to the
AssemblyManager or AssemblyConfiguration classes where we do
know the exact artifacts that'll be included in the container image.
Rolled-back checks in many of the generators where this logic wasn't applied correctly and some false positives would be logged in case of different packaging formats. In the future, we might want to consider adding the warning to the AssemblyManager or AssemblyConfiguration classes where we do know the exact artifacts that'll be included in the container image. Signed-off-by: Marc Nuri <marc@marcnuri.com>
Kudos, SonarCloud Quality Gate passed!
|
Description
Fixes #2070
Type of change
test, version modification, documentation, etc.)
Checklist