Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

AayushSaini101
Copy link
Contributor

@AayushSaini101 AayushSaini101 commented Sep 12, 2022

The new feature is added to the String comparison, now we can compare the values of the environment variable during the build time.

  1. Usage of the new feature: (We can compare the environment variables and we can compare this during the build process)
    new
  2. At present now, we don't have the functionality to compare values. I used a check box to allow users to compare environments with case-sensitive and without case-sensitive.
    new
  3. Console output:
    new

Copy link
Contributor

@MarkEWaite MarkEWaite left a 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) {
Copy link
Contributor

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.

Suggested change
public StringsMatchCondition(final String arg1, final String arg2, final boolean ignoreCase,final boolean shellVariables) {
public StringsMatchCondition(final String arg1, final String arg2) {

Copy link
Contributor Author

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;
Copy link
Contributor

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

Suggested change
final boolean shellVariables;
boolean shellVariables;

Copy link
Contributor Author

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.

new
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;
}

Copy link
Contributor

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.

Suggested change
@org.kohsuke.stapler.DataBoundSetter
public void setShellVariables(boolean value) {
shellVariables = value;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarkEWaite Okay Sir

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarkEWaite Done Sir

Comment on lines 74 to 75
final String expanded1 = TokenMacro.expandAll(build, listener, "$"+"{"+arg1+"}");
final String expanded2 = TokenMacro.expandAll(build, listener, "$"+"{"+arg2+"}");
Copy link
Contributor

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%?

Copy link
Contributor Author

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">
Copy link
Contributor

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?

Suggested change
<f:entry title="${%shellVariables}" field="enviornmentVariables">
<f:entry title="${%shellVariables}" field="environmentVariables">

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarkEWaite Done sir

@MarkEWaite MarkEWaite changed the title Feature updated to compare enviornment variables Feature updated to compare environment variables Sep 12, 2022
@AayushSaini101
Copy link
Contributor Author

AayushSaini101 commented Oct 8, 2022

@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" .

@AayushSaini101 AayushSaini101 requested a review from a team as a code owner June 1, 2024 16:24
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.
@MarkEWaite
Copy link
Contributor

@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:

  • c448c6c - undo incompatible change to constructor
  • 629bbb1 - use consistent formatting, reduce diffs to master branch

I see that you haven't yet implemented the two items that I requested earlier when I wrote:

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.

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.

Please implement those requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants