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

Re-design pullrequest managed by Updatecli #1245

Merged
merged 37 commits into from
Apr 8, 2023
Merged

Conversation

olblak
Copy link
Member

@olblak olblak commented Mar 24, 2023

Fix #1125

This pullrequest refactors the amount of information provided by Updatecli when it opens a pullrequest from an "action".
The goal is to be able to provide information for each target. Until now, Each pullrequest created by Updatecli would only use information retrieved from the first target. The reason is due to the fact the Updatecli can open/update the same pullrequest from different execution or pipeline, and it doesn't store any state. This made it difficult to identify when Updatecli had to update a pullrequest. To solve this problem, Updatecli store now pullrequest in html code such as

<details id="xxx">
<details>

where xxx is a sha256 has generated from manifest information.
If a pullrequest already exist, then it tries to parse the pullrequest body to retrieve already stored information.

An example of pullrequest is available on olblak/epinio-helm-chart#58

Test

To test this pull request, you can use any manifest that contains more that one target

cp <to_package_directory>
go test

Additional Information

Tradeoff

To generate ID, I mainly use resource title hash both. The risk of collision is very minimal.

A pullrequest now shows all target associated to a pipeline no matter that it was changed or not. The reason is because there is no state, there is no way to identify across Updatecli execution when an information was modified.

Potential improvement

  • I am planning to enriched PR content by using information changed by target, for that I need to update the target interface and I am planning to open a different pullrequest for that purpose only.

  • While looking at the gitea code, I noticed that scm relying on drone/go-scm doesn't support updating existing pullrequest. With the azure devops pullrequest, I realize that Updatecli needs are different, and only a subpart of drone/go-scm. I'll probably come with a similar but smaller library

  • Now the pullrequest body are a bit more complex, the action interface needs a dryrun function to just show what would change

  • Create action interface should be simplified

olblak and others added 16 commits March 11, 2023 08:46
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
…the implem

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Fix pointer

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Mar 25, 2023

Worth mentioning I decided to drop the following information

Source:
	✔ [chart] Get latest Rancher Helm Chart version(helmchart)


Condition:

Target:
	✔ [install] Update rancher/install.sh(file)
	✔ [manifest] Update rancher/manifest.yaml(yaml)

The reason is outside of showing the pipeline, the information is useless

@olblak
Copy link
Member Author

olblak commented Mar 25, 2023

While outside the scope of this PR, I am wondering if showing the pipeline using mermaidjs would be interesting

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
olblak and others added 5 commits March 26, 2023 11:46
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@lemeurherve
Copy link
Member

Do you have a pull request open somewhere with the current result?

I find olblak/epinio-helm-chart#52 too heavy and with many duplications, not sure if it's reflecting the current state of this PR.

@dduportal
Copy link
Contributor

dduportal commented Mar 27, 2023

Do you have a pull request open somewhere with the current result?

I find olblak/epinio-helm-chart#52 too heavy and with many duplications, not sure if it's reflecting the current state of this PR.
(edit)

@olblak can you provide screenshots? 😅 I concur with @hervelemeur

@olblak
Copy link
Member Author

olblak commented Mar 27, 2023

Let me reopen a few pullrequest, I did a lot of testing to cover different scenarios so I must admit that I polluated a lot my repository.

@dduportal
Copy link
Contributor

As per olblak/epinio-helm-chart#52, it looks unusable at all. It's not clear to me what is the intent.

Please don't merge this PR without a full discussion and agreement.

@olblak
Copy link
Member Author

olblak commented Mar 27, 2023

A simple example, with one pullrequest opened by one pipeline containing one target

raw html
<Actions>
    <action id="288309dfdc93af4a62c54e588aecc0383d215f0b34fbce9a0c10614df3798f8a">
        <h3>[updatecli] Bump golangci-lint version to v1.52.2</h3>
        <details id="37a8eec1ce19687d132fe29051dca629d164e2c4958ba141d5f4133a33f0688f">
            <summary>Update Golangci-lint version to v1.52.2</summary>
            <details>
                <summary>v1.52.2</summary>
                <pre>&#xA;Release published on the 2023-03-25 18:30:47 +0000 UTC at the url https://github.com/golangci/golangci-lint/releases/tag/v1.52.2&#xA;&#xA;## Changelog&#xA;* da04413a build(deps): bump github.com/moricho/tparallel from 0.3.0 to 0.3.1 (#3719)&#xA;* 1f4fed7c fix(pre-commit): require_serial &amp; pass_filenames (#3713)&#xA;&#xA;</pre>
            </details>
        </details>
    </action>
</Actions>

A pullrequest opened by seven pipelines (seven different manifest generated by autodiscovery)
olblak/epinio-helm-chart#58

some changes:

  • If the PR is updated after a new commit bumping once again to a newer version, the changelog will append the new version instead of replacing the existing one

@olblak
Copy link
Member Author

olblak commented Mar 27, 2023

Regarding the intend, two different pullrequests opened by the same manifest

**Currently, using version 0.47.2 **

-> olblak/epinio-helm-chart#55

the pullrequest body only mention one change done by the latest pipeline execution, and it will be overwritten all the time. The reason is Updatecli has no way to know if it was already executed

Proposal

olblak/epinio-helm-chart#58

List all changes no matter how many time we execute Updatecli. Updatecli stores "ID" in the pullrequest body to detect if it needs to update a sub-section

Please note that the constantly pullrequest title change is a bug which I need to fix in the autodiscovery

@dduportal
Copy link
Contributor

A simple example, with one pullrequest opened by one pipeline containing one target

olblak#222

That one is really nice and meet my desiderata ❤️

I'm trying a bit of exploration to explain my upcoming nitpicks on the design.

I'll also try to focus on the "multi actions" to help as much as I can.

First "draft": https://github.com/dduportal/updatecli/issues/27 (case of single dependency)

@olblak
Copy link
Member Author

olblak commented Mar 29, 2023

That one is really nice and meet my desiderata heart

Thanks for the feedback, I'll wait until the next weekend, and then I'll triggered a release with this change only.
Even thought I did a lot of testing, I would like to see how it behaves in the real world while being able to easily rollback

My main concern is to have a pullrequest that evolves over time with duplication such as olblak/epinio-helm-chart#52

Now that I think about it, I should probably also add the Updatecli version within the pullrequest metadata

@olblak
Copy link
Member Author

olblak commented Apr 2, 2023

Tested on Gitlab and Gitea and all works good

@olblak olblak changed the title More detailed pullrequest body Improve pullrequest body Apr 6, 2023
@olblak olblak changed the title Improve pullrequest body Re-design pullrequest managed by Updatecli Apr 8, 2023
@olblak olblak merged commit 454e7f8 into updatecli:main Apr 8, 2023
5 checks passed
@olblak olblak deleted the issue/1125 branch April 8, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Redesign GitHub Pullrequest body
3 participants