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

feat: add support for defining an --include file #210

Merged
merged 4 commits into from
Nov 24, 2021

Conversation

bbbco
Copy link
Contributor

@bbbco bbbco commented Nov 18, 2021

What does this pull request contains


  • Added for new features.
  • Changed for changes in existing functionality.
  • Deprecated for soon-to-be removed features.
  • Removed for now removed features.
  • Fixed for any bug fixes.
  • Security in case of vulnerabilities.

Explain your changes


My team has a use case for always requiring certain files to be included in the package.xml (these include a generated class file and test). And these files change based off of values (i.e. CI build numbers, etc) with each job run. We don't want to be submitting a new commit with each job build, but we need to include these files in the package.xml for source:deploy. Using this solution, we can ensure that the generated files are always included (along with git add -f).

Does this close any currently open issues?


closes #

  • Jest test to check the fix is applied are added.

Any particular element to being able to test locally


Any other comments?


Where has this been tested?


Operating System: Darwin Kernel Version 19.6.0: Tue Aug 24 20:28:00 PDT 2021; root:xnu-6153.141.40~1/RELEASE_X86_64

yarn version: 1.22.17

node version: v14.18.1

git version: git version 2.29.2

sfdx version: sfdx-cli/7.124.0 darwin-x64 node-v14.18.1

sgd plugin version: sfdx-git-delta 4.10.0 (link)

My team has a use case for always requiring certain files to be included in the package.xml (these include a generated class file and test). And these files change based off of values (i.e. CI build numbers, etc) with each job run. We don't want to be submitting a new commit with each job build, but we need to include these files in the package.xml for `source:deploy`. Using this solution, we can ensure that the generated files are always included (along with `git add -f`).
src/utils/repoGitDiff.js Outdated Show resolved Hide resolved
src/utils/repoGitDiff.js Outdated Show resolved Hide resolved
Copy link
Owner

@scolladon scolladon left a comment

Choose a reason for hiding this comment

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

👏 👍 the quality is great ! and the code is very close to be approved ! Bravo

I had a few remarks (following conventional comments style)
Here are the things I think should also be included

  • the new parameters should also be backported to the old command (bin/cli) (it will be deprecated soon)
  • IMO --include-destructive short parameter should be changed to '-N'
  • this PR should also test when the parameters are specified but the files does not exist (for -n and -N and also for -i and `-D')

I can help if you want to ! Add me to the fork

src/utils/cliHelper.js Show resolved Hide resolved
src/utils/repoGitDiff.js Show resolved Hide resolved
src/utils/repoGitDiff.js Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/commands/sgd/source/delta.ts Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/utils/cliHelper.js Show resolved Hide resolved
src/utils/repoGitDiff.js Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #210 (d7de74b) into master (04470f9) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #210   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines          482       498   +16     
=========================================
+ Hits           482       498   +16     
Impacted Files Coverage Δ
src/main.js 100.00% <100.00%> (ø)
src/utils/cliHelper.js 100.00% <100.00%> (ø)
src/utils/repoGitDiff.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04470f9...d7de74b. Read the comment docs.

@scolladon
Copy link
Owner

Hi @bbbco !

Thanks for this great contribution !
The quality is very good ! And it brings value !
Thank you very much !

I made a review of this work, we can discuss in the thread for each items.

Overall I have a questions:

  • What was your developer experience with the project ?
  • How easy/hard was it for you to dive into the code ?
  • How was your experience with all the tooling and checks we added for DevOps ?
  • Basically, how can we make this project better according to you and your experience here ?

@scolladon scolladon added the good first issue Good for newcomers label Nov 18, 2021
@bbbco
Copy link
Contributor Author

bbbco commented Nov 19, 2021

Thanks for the great review!

I just pushed up the changes requested.

Have a great weekend!

scolladon
scolladon previously approved these changes Nov 19, 2021
Copy link
Owner

@scolladon scolladon left a comment

Choose a reason for hiding this comment

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

LGTM!

Great job here, thanks @bbbco and bravo

mehdicherf
mehdicherf previously approved these changes Nov 22, 2021
Copy link
Collaborator

@mehdicherf mehdicherf left a comment

Choose a reason for hiding this comment

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

Excellent contribution @bbbco , it's a great addition to the plugin, thank you!

@bbbco
Copy link
Contributor Author

bbbco commented Nov 22, 2021

Glad to help! What do we need to do to fix the pr-lint check?

@scolladon
Copy link
Owner

Nothing, it is because the PR is from another repository, you don't have the token required to execute the job

The PR title is following the guidelines

@bbbco bbbco dismissed stale reviews from mehdicherf and scolladon via d7de74b November 24, 2021 15:27
@codeclimate
Copy link

codeclimate bot commented Nov 24, 2021

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

View more on Code Climate.

Copy link
Owner

@scolladon scolladon left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

3 participants