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

Expand environment variables for sparse checkout path #1066

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

Conversation

raghavgarg098
Copy link
Contributor

@raghavgarg098 raghavgarg098 commented Apr 8, 2021

JENKINS-23477

Expand environment variables in sparse checkout path using pattern matching to look for environment variables name.

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • Documentation in README has been updated as necessary
  • Online help has been added and reviewed for any new or modified fields
  • I have interactively tested my changes
  • Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)


public class SparseCheckoutPath extends AbstractDescribableImpl<SparseCheckoutPath> implements Serializable {

private static final long serialVersionUID = -6177158367915899356L;

@SuppressFBWarnings(value="SE_TRANSIENT_FIELD_NOT_RESTORED", justification="Default value is OK in deserialization")
public static final transient SparseCheckoutPathToPath SPARSE_CHECKOUT_PATH_TO_PATH = new SparseCheckoutPathToPath();
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this change break compatibility for git plugin API consumers that refer to this public field that is being deleted?

It is included in the Javadoc of the current release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to keep this variable for API consumers.

public String apply(@NonNull SparseCheckoutPath sparseCheckoutPath) {
return sparseCheckoutPath.getPath();
}
public Descriptor<SparseCheckoutPath> getDescriptor() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't make white space changes in the git plugin. The formatting is ugly and inconsistent, but changing white space makes it more difficult to read the changes and more difficult to interact with other proposed changes in the same file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why the getDescriptor() call moved outside SparseCheckoutPathToPath. What makes that move necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not intentional. Somehow changed on reformating the code in IDE.

}

@Extension
public static class DescriptorImpl extends Descriptor<SparseCheckoutPath> {
@Override
public String getDisplayName() { return "Path"; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't make white space only changes in the git plugin. See the contributing file for more details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned this was not intentional either.

Comment on lines 54 to 61
@Extension
public static class DescriptorImpl extends GitSCMExtensionDescriptor {
@Override
public String getDisplayName() {
return "Sparse Checkout paths";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why moving so many methods outside the descriptor implementation class? Seems like that will break many things and add unused public methods to the SparseCheckoutPaths object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same

@@ -67,11 +68,11 @@ public boolean equals(Object o) {
if (this == o) {
return true;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not able to resolve these issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not able to revert these changes.

@raghavgarg098 raghavgarg098 changed the title Sparse checkout Expand environment variables for sparse checkout path Apr 8, 2021
@MarkEWaite MarkEWaite added the enhancement Improvement or new feature label Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants