-
Notifications
You must be signed in to change notification settings - Fork 29
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
Feature updated to compare environment variables #30
base: master
Are you sure you want to change the base?
Feature updated to compare environment variables #30
Conversation
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.
Thanks for the pull request.
I'm not sure of the use case that this is addressing. Can you add a documentation proposal for text in the README? That is a good place to describe how it will be used and how it will help users.
If this is fixing a specific issue report, please include a link to the issue report in the pull request description.
Please add automated tests that confirm the modified code is behaving as expected. An enhancement without automated tests is much more likely to be harmed by later changes than an enhancement that includes automated tests.
|
||
@DataBoundConstructor | ||
public StringsMatchCondition(final String arg1, final String arg2, final boolean ignoreCase) { | ||
public StringsMatchCondition(final String arg1, final String arg2, final boolean ignoreCase,final boolean shellVariables) { |
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.
Public APIs in Jenkins are usually treated as stable so that we don't break compilation of consumers of the public API. In this case, rather than adding the argument to the DataBoundConstructor
, I think that you want to define a DataBoundSetter that can define the value after the object has been created.
public StringsMatchCondition(final String arg1, final String arg2, final boolean ignoreCase,final boolean shellVariables) { | |
public StringsMatchCondition(final String arg1, final String arg2) { |
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.
@MarkEWaite Done sir
public class StringsMatchCondition extends AlwaysPrebuildRunCondition { | ||
|
||
final String arg1; | ||
final String arg2; | ||
final boolean ignoreCase; | ||
final boolean shellVariables; |
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.
Don't make it final
so that it can be set from the DataBoundSetter
final boolean shellVariables; | |
boolean shellVariables; |
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.
Thanks for the pull request.
I'm not sure of the use case that this is addressing. Can you add a documentation proposal for text in the README? That is a good place to describe how it will be used and how it will help users.
If this is fixing a specific issue report, please include a link to the issue report in the pull request description.
Please add automated tests that confirm the modified code is behaving as expected. An enhancement without automated tests is much more likely to be harmed by later changes than an enhancement that includes automated tests.
Sir, In the current version, there are no test cases is present as of now related to StringMatchCondition.java
this.arg1 = arg1; | ||
this.arg2 = arg2; | ||
this.ignoreCase = ignoreCase; | ||
this.shellVariables=shellVariables; | ||
} | ||
|
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.
Use a DataBoundSetter
rather than modifying the DataBoundConstructor
so that the signature of the constructor does not change.
@org.kohsuke.stapler.DataBoundSetter | |
public void setShellVariables(boolean value) { | |
shellVariables = value; | |
} |
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.
@MarkEWaite Okay Sir
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.
@MarkEWaite Done Sir
final String expanded1 = TokenMacro.expandAll(build, listener, "$"+"{"+arg1+"}"); | ||
final String expanded2 = TokenMacro.expandAll(build, listener, "$"+"{"+arg2+"}"); |
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.
This seems like it only handles the Unix environment variables expansion. Aren't Windows environment variables represented as well? Something like %MY_VAR%
?
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, Sir they are handled
@@ -25,6 +25,9 @@ | |||
|
|||
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form"> | |||
|
|||
<f:entry title="${%shellVariables}" field="enviornmentVariables"> |
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.
Spelling errors last a very long time in identifiers. Could you correct this one?
<f:entry title="${%shellVariables}" field="enviornmentVariables"> | |
<f:entry title="${%shellVariables}" field="environmentVariables"> |
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.
@MarkEWaite Done sir
.../resources/org/jenkins_ci/plugins/run_condition/core/StringsMatchCondition/config.properties
Outdated
Show resolved
Hide resolved
@MarkEWaite Sir can you please review this PR? It is pending for the last 25 days, or should I make another condition to add this feature to the existing plugin that would be "comparingEnviornmentVariables" . |
My suggestion was applied but the suggestion was wrong. Sorry about that. The constructor should remain the same so we don't surprise consumers.
4 space indentation is used in the rest of the file, let's continue using it in this case. Clarify what is changing by reducing the diffs to the master branch.
@AayushSaini101 thanks for the changes. I've reviewed your changes and pushed two additional changes to the pull request. Please review the comments that I made in the commits:
I see that you haven't yet implemented the two items that I requested earlier when I wrote:
Please implement those requests. |
The new feature is added to the String comparison, now we can compare the values of the environment variable during the build time.