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

Enable pre/post destructiveChanges manifests #583

Open
AllanOricil opened this issue Apr 29, 2023 · 8 comments
Open

Enable pre/post destructiveChanges manifests #583

AllanOricil opened this issue Apr 29, 2023 · 8 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@AllanOricil
Copy link
Contributor

AllanOricil commented Apr 29, 2023

Is your proposal related to a problem?

I can't determine if the changes generated by the plugin should be deleted before, after or in the same deployment job. I think that the solution for this problem is to include "reserved keywords" in git messages that sgd could use. Why version this information? Because I could replay the branch history to determine the order metadata changes happened, including deletions.

Describe a solution you propose

Given this set of commits

23echsd      [sgd-delete-before] commit message 1
84jdhe2       commit message 2
528jded       [sgd-delete-after] commit messages 3

and considering HEAD is pointing to 528jded

sfdx sgd:source:delta --from 23echsd

could output 4 manifests

preDestructiveChanges.xml
postDestructiveChanges.xml
destructiveChanges.xml
package.xml

This feature should not need a feature flag to work, in my opinion.

Then this proposal #558 could be extended to include new parameters in the output

{
    "has_pre_destructive_changes": true,
    "has_post_destructive_changes": true,
    "has_destructive_changes": true,
    "has_package_changes": true
}

Describe alternatives you've considered

Currently there is no alternative. I would have to read commits again and run sgd multiple times between every commit if I create my own solution.

Additional context

N/A

@AllanOricil AllanOricil added the enhancement New feature or request label Apr 29, 2023
@AllanOricil
Copy link
Contributor Author

AllanOricil commented Apr 29, 2023

Or maybe there is another way you see people doing this kind of automation. I imagine that they use PR comments, and I don't think this is good strategy because the order things are added or removed gets decoupled from the history of changes

@AllanOricil
Copy link
Contributor Author

AllanOricil commented Apr 29, 2023

There could also exist a keyword to mark deployment job boundaries. This way we can create some sort of deployment in multiple steps using commits:

something like [sgd-delta-boundary]

23echsd     [sgd-delete-before] commit message 1
84jdhe2       commit message 2
528jded      [sgd-delta-boundary] [sgd-delete-after] commit messages 3
63ecdsd     [sgd-delete-before] commit message 1
94jwhe2       commit message 2
a28jded      [sgd-delta-boundary] [sgd-delete-after] commit messages 3

When multiple [sgd-delta-boundary] are found, sgd will generate multiple folders to separate the manifests.

delta_boundary_1
     preDestructiveChanges.xml
     postDestructiveChanges.xml
     destructiveChanges.xml
     package.xml
delta_boundary_2
     preDestructiveChanges.xml
     postDestructiveChanges.xml
     destructiveChanges.xml 
    package.xml

The json output would have information that can easily help us to determine how many deployments sgd created

@scolladon
Copy link
Owner

Hi @AllanOricil !

Thanks for raising this enhancement request and thanks for contributing in making this project better!

Could you create another Enhancement Request for the "deployment job boundaries" concept?
This way we will be able to work on them separately.

I'm very interested in both concepts.
I think it can be very useful for some very sharp DevOps situations.
I cannot imagine easily what those DevOps workflow could be and if there is no simpler design to address the issue it addresses, but I'm interested in the concepts :)
I'll think about this in the next following days, what use cases it enables, what problems it solves, is there other suitable ways to approach this, and see what would be a suitable software design to easily ship this.

For the destructive document part, why do you think we should distinguish before from same ? To me a destructiveChanges.xml is always applied before deployment. So I think we could consider after tag only. If the commit is not tagged then it should be added in the destructiveChangesPre.xml (nominal use case).
In the end we would have the package.xml, the destructiveChangesPre.xml (instead of the destructiveChanges.xml) and the destructiveChangesPost.xml.
Do you have any specific use case using 3 destructiveChanges? What is your idea behind keeping the destructiveChanges.xml?

@AllanOricil
Copy link
Contributor Author

AllanOricil commented Apr 30, 2023

For the destructive document part, why do you think we should distinguish before from same ? To me a

Yes, I thought that destructiveChanges.xml would run in the same deployment job. But according to the docs, it runs before.

So I think we could consider after tag only. If the commit is not tagged then it should be added in the destructiveChangesPre.xml (nominal use case).

Yes, there is no need for using other keywords besides after

Do you have any specific use case using 3 destructiveChanges? What is your idea behind keeping the destructiveChanges.xml?

I don't. It wouldn't be necessary.

@scolladon scolladon added the good first issue Good for newcomers label May 4, 2023
@scolladon
Copy link
Owner

What I understand from the need

The need is to have a way to deal with pre and post destructive content, driven by the repository.
By default (when not splitted), content in the destructiveChanges.xml is executed before.
sfdx-git-delta should be able to deduce where to put deleted element (in pre or in post) from reading the git repository.

What I understand from the solution proposed

Any deletion should be added in the destructiveChangesPre.xml (it works the same way as if it was in the destructiveChanges.xml)
When a deletion is in a commit flagged with a keyword ([sgd-delete-before] for instance) in the commit message (or git note) then the deletion is added in the destructiveChangesPost.xml

Edge cases discussion

I can see the value in CI when evaluating a PR.
In this context we can have multiple commits against the target branch.
It is easy to then only validate the delta with the target branch and the diff will allow to move the deleted element in pre or post destructiveChanges.xml.
But, when this PR is merged it will be one commit in the target branch (when merged with squash and merge)... If we want to do the same deployment into a QA environment for example when it is merged in the target branch then it will not be possible to have this granularity and to know what to scope to put in the destructiveChangesPost.xml.
So I think this solution would not work for this kind of workflow and would force the user in a specific workflow to benefit from this capability.

Alternative approach

I don't have one so far... 😅
I really like reading the commit message, it is easy, even if the actual architecture is not suitable for this kind of work I think it could be done in post processing for example.
But I don't know how we could have a solution to detect when a change needs to be put into the destructiveChangesPost.xml based on git content, for any usage...

Let's think about it further, maybe @AllanOricil you'll get a good idea on how to detect what metadata to put in destructiveChangesPost.xml whatever the kind of commit (squash / merge / range)

@AllanOricil
Copy link
Contributor Author

AllanOricil commented May 6, 2023

What if sgd exposes a new flag that accepts a list of metadata paths that have to be deleted using destructiveChangesPost.xml. CICD pipelines can fetch this information from PR descriptions or commit messages, and pass it to sgd using a certain format that the cli would be expecting.

This flag could be called --delete-after:

sgd ... --delete-after ./package-a/main/default/classes/Foo.cls --delete-after ./package-b/main/default/classes/Bar.cls ...

sgd would need to take care of a few things:

  1. verify if all paths passed to --delete-after flags were really deleted. If at least 1 given file path isn't deleted according to git diffs, display an error and exit 1. We don't want to accidentally delete things that were not deleted.
  2. generate both destructive changes manifests, but If all deleted files paths were passed to --delete-after, skip the creation of the destructiveChangesPre.xml
  3. deleted paths that were not passed to --delete-after must be added to destructiveChanges.xml if there is no metadata to be deleted after, or to destructiveChangesPre.xml if there is a list of metadata to be deleted after.
  4. send the following information to stdout. It is easier to parse a json than count the number of metadata types in a given xml manifest to determine if we should or not schedule a deploy job.
{
    "has_pre_destructive_changes": true|false,
    "has_post_destructive_changes": true|false,
    "has_destructive_changes": true|false,
    "has_package_changes": true|false
}

@AllanOricil AllanOricil changed the title Enable pre/post/same destructiveChanges using keywords in commit messages Enable pre/post destructiveChanges manifests May 6, 2023
@AllanOricil
Copy link
Contributor Author

AllanOricil commented May 8, 2023

It is the same as if we were using Salesforce's UI to configure which metadata items have to be deleted afterwards

https://success.flosum.com/s/article/Destructive-Changes

The UI in this case would be repository's PR description/comment. The input would be parsed to a format that sgd wants.

@scolladon
Copy link
Owner

Yep I agree @AllanOricil, It is the same as we were using a graphical UI instead of a CLI here.

I think point 4 here is debatable (just saying 🙂)

I like the --delete-after, it seems to address the issue of the squash and merge commit making disappear the information when the information is read from the commit message but the UX is does not look very convenient yet.
But it looks like it could be done by versioning a destructiveChangesPost.xml and remove its content from the destructiveChanges(Pre).xml via a simple command / tool (like sed).
The advantage for the parameter would be it does not require to empty the destructiveChangesPost.xml after a release.

I'll try to think on that in the coming days, and let you know if I have an idea.
Feel free to improve if you have inspiration 🙂

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

No branches or pull requests

2 participants