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: show fine-grained detail of changes in npx checkly deploy --preview #527

Open
clample opened this issue Feb 15, 2023 · 11 comments
Open
Labels
enhancement New feature or request

Comments

@clample
Copy link
Collaborator

clample commented Feb 15, 2023

What problem does this feature solve?

This issue relates to #526, but I think that it's a distinct issue and a bit harder to fix.

Currently npx checkly deploy --preview only shows high-level information about the changes: which checks/resources were created, updated, and deleted. It currently doesn't show which fields are changed. For example, it won't show that a check goes from activated: true to activated: false. It also doesn't detect the case where a check isn't changed at-all - the check will still show as updated in the diff.

It would be great if npx checkly deploy --preview gave more fine-grained detail:

$ npx checkly deploy --preview

Deleted:
    (Check) 'My Check'
    
Updated:
    (Check) 'Other Check'
        + activated: true ~> false
        + frequency: 10 ~> 15
        
Unchanged:
    (Check) 'another check'

How would you implement this feature?

Maybe jest-diff is useful. We can also look at terraform plan for inspiration on how the output should be formated.

@clample clample added the enhancement New feature or request label Feb 15, 2023
@nahuelon nahuelon self-assigned this Feb 16, 2023
@nahuelon
Copy link
Contributor

@clample , I was trying a package deep-object-diff to get the added, updated and deleted dif. I'll continue working in the /next/deploy endpoint, adding a detailed JSON we could manipulate in the CLI to display user-friendly response. Maybe it's related with the issue #526 you are working on.

@nahuelon nahuelon removed their assignment Feb 16, 2023
@clample
Copy link
Collaborator Author

clample commented Feb 20, 2023

Thanks for checking this out @nahuelon. deep-object-diff looks useful.

The two issues are really closely related. We could maybe even tackle them both at once and just completely rework the --preview.

I created two issues since I want to have the --preview output cleaned up in a human readable form before the launch week. I'm not sure that we'll be able to finish all of the changes to /next/deploy in time, though, so I created #526 to clean up the output just using the data that we already have.

@tnolet
Copy link
Member

tnolet commented Feb 23, 2023

@clample can we close this one?

@clample
Copy link
Collaborator Author

clample commented Feb 23, 2023

@tnolet personally I think that it would be a useful feature. It would take a day or two to do, though.

Please feel free to close it if you don't think that it's worth doing.

@tnolet
Copy link
Member

tnolet commented Feb 23, 2023

@clample I mixed this up with the other ticket. We can leave this open. I love the output suggested here.

@nahuelon nahuelon self-assigned this Feb 27, 2023
@tnolet tnolet added this to the Checkly CLI GA milestone Feb 27, 2023
@nahuelon
Copy link
Contributor

nahuelon commented Mar 3, 2023

@clample @umutuzgur , I finished a first approach displaying a the differences with json-diff. I still need to improve some changes a applied in the backend, but I wanted to show the progress of the --preview output to get your feedback.

Diff Output

@clample
Copy link
Collaborator Author

clample commented Mar 3, 2023

@nahuelon that looks awesome 👀

Should/could we add special handling for browser check code? In your example screenshot for change-1, it could be cool to just show a text diff of the exact line in script that changes. For the output, I imagine that this would look something like git diff. Just applying the json-diff, it looks like the whole script will be changed. I'm not sure exactly how it would look in the end, though. What do you think? Maybe it's worth merging with just the json-diff, though, and we can iterate later on.

@clample
Copy link
Collaborator Author

clample commented Mar 3, 2023

@tnolet maybe you have ideas on how --preview should work?

@tnolet
Copy link
Member

tnolet commented Mar 3, 2023

@clample @nahuelon the proposal here looks good, I don't think we can fully design up front what will be the most useful layout. Two niggles with the proposed layout:

  • I think in CRUD, where the Read is irrelevant: Created, Updated, Deleted. We do not show Created
  • I'm missing a summary at the bottom. When doing small changes, on one or two files, I just want to get that quick confirmation I did not screw something up. terraform plan adds a small summary, I added it also to the example below
  • The unchanged list can get very long and is not a big concern. We can remove that.
  • The order of the categories should always be: Created, Updated, Deleted.
$ npx checkly deploy --preview

Created:
    (Check) 'another check'

Updated:
    (Check) 'Other Check'
        + activated: true ~> false
        + frequency: 10 ~> 15

Deleted:
    (Check) 'My Check'
   
summary:  1 to create, 1 to update, 1 to delete

@nahuelon
Copy link
Contributor

nahuelon commented Mar 3, 2023

@umutuzgur @clample , we'll need to apply some changes in the backend to be able to compare the current vs. next state. I was able to 'simulate' the new state by using a transaction, apply all the changes, get the new state and rollback the transaction. Of course, this is not something we can use in production (i.e. one problem would be the serial id are increased on each transaction).
One potential solution might be to add a new column (i.e. state) in the projects table with all the JSON used in the deploy. Having that, we could compare the sent payload with the current state JSON when the --preview is set. Comparing the CLI project generated payload with the project graph is not a possibility because the graph includes lot of properties added in the check/request/alerts which weren't sent from the CLI.
I'll continue by adding the state column and see if it works as I'm thinking.
Then, having two comparable JSON structure, I'll be able to generate the output suggested in the previous message.

@umutuzgur
Copy link
Member

@nahuelon @clample I talked to @tnolet about this one. This ticket has low priority given we have tight schedule for delivery. Let's tackle this as the last ticket when every ticket is tackled.

@nahuelon nahuelon removed their assignment Mar 13, 2023
@tnolet tnolet removed this from the Checkly CLI GA milestone May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants