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

fix: check for commit ref for --to and --from parameters #985

Merged
merged 14 commits into from
Jan 16, 2025

Conversation

scolladon
Copy link
Owner

@scolladon scolladon commented Jan 11, 2025

Explain your changes


Fixes an issue with how rev-parse was used to check --to or --from parameters content

Before it was possible to pass "." or ".." to either --to or --from. Our rev-parse check did not return errors but was not returning parsed revision as well.
This could lead to very strange behaviour and could mess with inFile comparison and "include" feature handling.
Now the plugin uses the parameter --verify for the rev-parse command which checks only for revision or alias of revision

Improves how error are communicate to end users.

Before error where not displayed when not using --json
Leaving the user interpret the result based on the exit code (not the most user friendly)
Now the plugin display the error with its message in the end or the success !

Improves CI speed

Disable SF CLI autoupdate and telemetry

@scolladon scolladon requested a review from mehdicherf as a code owner January 11, 2025 20:42
@scolladon scolladon changed the title feat: allow to also compare with uncommited work feat: allow to also compare with uncommitted work Jan 11, 2025
Copy link

codecov bot commented Jan 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (769ce5f) to head (4c18b7a).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #985   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           44        44           
  Lines          990       994    +4     
  Branches       103       105    +2     
=========================================
+ Hits           990       994    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@scolladon scolladon force-pushed the fix/head-to-parameters branch from 39cb93c to 00cea45 Compare January 11, 2025 20:46
@scolladon
Copy link
Owner Author

You may want to test this @nvuillam
Maybe your users will be impacted as this PR propose to include uncommitted work by default when --to parameter is not specified (or is specified with "")
Your opinion will be highly valuable as well, feel free to speak your mind !

@scolladon
Copy link
Owner Author

You can follow those steps to easily test this PR locally

@scolladon scolladon force-pushed the fix/head-to-parameters branch from 0126c2c to 519f89c Compare January 11, 2025 21:21
@nvuillam
Copy link
Contributor

You may want to test this @nvuillam
Maybe your users will be impacted as this PR propose to include uncommitted work by default when --to parameter is not specified (or is specified with "")
Your opinion will be highly valuable as well, feel free to speak your mind !

Sfdx-hardis always uses --to argument so if you don't change its behavior when a value is sent, it's not a breaking change so ok for me 😊

@nvuillam
Copy link
Contributor

Additional note: if it changes the behavior when --to is not sent, if u respect semver your next release will be 7.0.0 😊

@scolladon scolladon force-pushed the fix/head-to-parameters branch from b123525 to 8584e86 Compare January 11, 2025 23:43
@scolladon
Copy link
Owner Author

Additional note: if it changes the behavior when --to is not sent, if u respect semver your next release will be 7.0.0 😊

That's right, and it would have been the case with the previous version of this PR.
I rolledback the previous default behaviour and just allow to pass "" as a parameter if one wants to include uncommited work as well.
Enough breaking change already for january ;)
Thanks @nvuillam !

@scolladon scolladon changed the title feat: allow to also compare with uncommitted work feat: compare with uncommitted work Jan 11, 2025
@scolladon
Copy link
Owner Author

Quick follow up here of your tests @thomas100z

Can you tell me how to reproduce your remarks here ("new file are not added")
Just to be sure to understand properly your POV

@thomas100z
Copy link

What I was doing comes down to these steps:

  1. Make new CustomField in sandbox
  2. Retrieve CustomField metadata from sandbox
  3. Generate delta package with the dev-985 version with command sf sgd source delta --to "" --from "origin/staging" --output-dir changed-sources/ --source-dir force-app/
  4. Observe new CustomeField not in changed-sources/package/package.xml

@scolladon
Copy link
Owner Author

scolladon commented Jan 13, 2025

Unfortunately it is not possible to git diff with content not even staged...
To have them included you have to add those files in the git staging area (git add ...).

This makes me wonder if we really need this or if just using the workaround to commit before running sgd would be enough.

With the current change on --to
If you want to compare commits : use sgd as usual
If you want to compare commits and include changes not commited : use sgd with "" for --to
If you want to compare commits and include changes not commited and new fils : git add new files and use sgd with "" for --to

-> Unstage new files if not needed finally

Without the current change on --to
If you want to compare commits : use sgd as usual
If you want to compare commits and include changes not commited : git add and git commit and sgd as usual (reset commit if needed)
If you want to compare commits and include changes not commited and new fils : git add and git commit and sgd as usual (reset commit if needed)

-> Soft reset in the end, instead of unstaging new files

Your opinion about this @mehdicherf ?

@scolladon scolladon force-pushed the fix/head-to-parameters branch from 8584e86 to 82b091b Compare January 14, 2025 15:27
@mehdicherf
Copy link
Collaborator

mehdicherf commented Jan 15, 2025

@scolladon My 2 cents:

I didn't even think about that use case where --to is not specified.
The tool was primarily designed to help with deployments, where anyway we only deal with metadata committed to the repo.

My personal opinion is that there is little value in offering a diff to staged (because as you mentioned, there needs to be a step to stage the files, and currently the user can simply do an additional commit (and reset later) if needed.

That being said, since you already did the job, I don't have see any objections either to include this feature in the plugin, as long as it's not a breaking change for existing situations where --to is specified (my understanding is that this behaviour is unchanged, and I'm good with it!)

@scolladon
Copy link
Owner Author

Ok I will roll back the new feature to take the uncommitted work.

This PR and the related discussions were very helpful.
It helped ensure the goal of SGD is to compare code base committed in the repository (code we actually validate and want to be deployed)

It also helped find a way to deploy uncommitted changes and new files:

  • commit them, use sgd, then reset (soft if you want to keep the changes) the commit
  • create a branch locally, commit them, use sgd, then delete the branch

@scolladon scolladon changed the title feat: compare with uncommitted work fix: check for commit ref for --to and --from parameters Jan 15, 2025
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 6827 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 6836 lines exceeds the maximum allowed for the inline comments feature.

Copy link

codeclimate bot commented Jan 16, 2025

Code Climate has analyzed commit 4c18b7a and detected 0 issues on this pull request.

View more on Code Climate.

@scolladon scolladon merged commit 4768ed8 into main Jan 16, 2025
18 of 20 checks passed
@scolladon scolladon deleted the fix/head-to-parameters branch January 16, 2025 13:31
Copy link

Shipped in release v6.1.1.
Version v6.1.1 will be assigned to the latest npm channel soon
Install it using either v6.1.1 or the latest-rc npm channel

$ sf plugins install sfdx-git-delta@latest-rc
# Or
$ sf plugins install [email protected]

💡 Enjoying sfdx-git-delta?
Your contribution helps us provide fast support 🚀 and high quality features 🔥
Become a sponsor 💙
Happy incremental deployment!

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.

4 participants