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(cli): expanded preview #267

Merged
merged 40 commits into from
Aug 9, 2021
Merged

feat(cli): expanded preview #267

merged 40 commits into from
Aug 9, 2021

Conversation

louis-bompart
Copy link
Collaborator

@louis-bompart louis-bompart commented Jun 17, 2021

Proposed changes

When previewing, being it thru the dedicated command or part of the push flow allow the user to analyze files to preview what's what.

To be precise:

  • We 'pull' the target organization configuration (i.e. create a new snapshot from org, pull it).
  • We compare each file using the resourceKeys.
    • Resource key present in the target but not in the source snapshot are added
    • Resource key present in the source snapshot but not in the target are kept unless using the -d flag.
    • If present in both, then the value of the resource from the source is applied on the target.

Testing

The focus of the tests will be on UT for this, testing is still ongoing:

  • ExpandedPreviewer
  • FileDiffProcessor

CDX-347

@louis-bompart louis-bompart marked this pull request as ready for review July 22, 2021 20:13
@louis-bompart louis-bompart changed the title expanded preview, early sneakpeak WIP feat(cli): expanded preview Jul 22, 2021
Copy link
Member

@olamothe olamothe left a comment

Choose a reason for hiding this comment

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

@louis-bompart : Are these changes ready to review and merge ?

I saw your last comment about the backend not being ready and you preferring to wait. If that's the case, can you close this temporarily and re-open when it's ready (or switch it to draft).

Thanks !

@louis-bompart
Copy link
Collaborator Author

@louis-bompart : Are these changes ready to review and merge ?

@olamothe, the PR description is up-to-date with the work that I think needs to be done (mainly, UT), but it is ready for review.

I saw your last comment about the backend not being ready and you preferring to wait. If that's the case, can you close this temporarily and re-open when it's ready (or switch it to draft).

I don't know which comment you are talking about 😕?

@olamothe
Copy link
Member

I don't know which comment you are talking about 😕?

This one:

And honestly, I don't think I would merge it till I can test it for real. (the more I coded it, the more I realized that there are a lot of places where it can go haywire, so I don't feel comfortable going forward till the backend is done and that I'm able to see what my code does.

But if it's ready, then 👍 Ill do the review.

packages/cli/src/lib/project/project.ts Outdated Show resolved Hide resolved
packages/cli/src/lib/project/project.ts Outdated Show resolved Hide resolved
packages/cli/src/lib/snapshot/expandedPreviewer.ts Outdated Show resolved Hide resolved
packages/cli/src/lib/snapshot/expandedPreviewer.ts Outdated Show resolved Hide resolved
packages/cli/src/lib/snapshot/expandedPreviewer.ts Outdated Show resolved Hide resolved
@louis-bompart
Copy link
Collaborator Author

louis-bompart commented Jul 29, 2021

Ready for full review. Incorporate part of #391 and needs it to be merged first

Copy link
Contributor

@y-lakhdar y-lakhdar left a comment

Choose a reason for hiding this comment

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

image

There is a point in the terminal where nothing happens while the expanded preview is being computed. Maybe a spinner or any kind of feedback would let the user know this is normal

Copy link
Contributor

@y-lakhdar y-lakhdar left a comment

Choose a reason for hiding this comment

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

GG.
Maybe another to add a spinner while computing the expanded preview

@louis-bompart louis-bompart merged commit 69262c1 into master Aug 9, 2021
@louis-bompart louis-bompart deleted the CDX-347 branch August 9, 2021 18:08
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.

3 participants