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(action): show CI url in pullrequest body #1737

Merged
merged 13 commits into from
Nov 5, 2023
Merged

Conversation

olblak
Copy link
Member

@olblak olblak commented Nov 3, 2023

Fix #1717

Test

Well I don't see how to test this feature without taking the risk of conflicting with the GitHub Action environment used to test Updatecli so I am just doing manual testing at this stage.

We must use an Updatecli that need to change something

export GITHUB_ACTION="true" 
export GITHUB_SERVER_URL="https://github.com" 
export GITHUB_REPOSITORY="foo/bar" 
export GITHUB_RUN_ID="1658821493" 

./bin/updatecli diff --config updatecli/updatecli.d/venom.yaml --debug

Please note <a href="https://github.com/foo/bar/actions/runs/1658821493">GitHub Action pipeline link</a> in the console output

Console Output
DEBUG: The expected action would have the following information:
	|	
	|	##Title:
	|	ci: bump Venom version to v1.0.1
	|	
	|	
	|	##Report:
	|	
	|	<Action id="1c70af8de95bd7989d99731b82355dfdd0fbd5981ebfef645550823023b51711">
	|	    <h3>ci: bump Venom version</h3>
	|	    <details id="dc3a5f47e02ede555033ee1e6cb765e5e88ea60e3352a90edb40cb190d142911">
	|	        <summary>ci: update Venom version to v1.0.1</summary>
	|	        <p>1 file(s) updated with &#34;VENOM_VERSION: v1.0.1&#34;:&#xA;&#x9;* .github/workflows/go.yaml&#xA;</p>
	|	        <details>
	|	            <summary>v1.0.1</summary>
	|	            <pre>&#xA;Release published on the 2021-12-13 16:52:12 +0000 UTC at the url https://github.com/ovh/venom/releases/tag/v1.0.1&#xA;&#xA;# Venom v1.0.1&#xD;&#xA;&#xD;&#xA;## What&#39;s Changed&#xD;&#xA;* fix: handle json number is assertions (#460) https://github.com/ovh/venom/pull/464&#xD;&#xA;&#xD;&#xA;**Full Changelog**: https://github.com/ovh/venom/compare/v1.0.0...v1.0.1</pre>
	|	        </details>
	|	    </details>
	|	    <a href="https://github.com/foo/bar/actions/runs/1658821493">GitHub Action pipeline link</a>
	|	</Action>
	|	
	|	=====
	|	

=============================

This pullrequest introduces in a new action setting pipelineurl such as

actions:
    default:
        title: 'ci: bump Venom version to {{ source "latestVersion" }}'
        kind: github/pullrequest
        disablepipelineurl: true
        spec:
            automerge: true
            labels:
                - chore
                - skip-changelog
        scmid: default

Additional Information

Tradeoff

Potential improvement

  • I need to gate this feature behind a setting at the action level

Signed-off-by: Olblak <me@olblak.com>
@olblak olblak added the enhancement New feature or request label Nov 3, 2023
@olblak olblak marked this pull request as draft November 3, 2023 16:54
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 olblak marked this pull request as ready for review November 3, 2023 19:31
Signed-off-by: Olblak <me@olblak.com>
dduportal
dduportal previously approved these changes Nov 4, 2023
Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

LGTM. I've added a few nitpicks, you're free to select them or not.

The last one (duplication of variables) is NOT a blocker. IF you want me to send a PR,n that will be a good exercise for me to put my hand dirty in updatecli again (haven't done it since a long time)

pkg/core/reports/action.go Outdated Show resolved Hide resolved
pkg/core/reports/action.go Show resolved Hide resolved
pkg/core/reports/action.go Show resolved Hide resolved
pkg/core/reports/action.go Outdated Show resolved Hide resolved
pkg/core/reports/action.go Show resolved Hide resolved
olblak and others added 4 commits November 4, 2023 14:19
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal
Copy link
Contributor

I wonder if the pipelineurl shouldn't be true by default (e.g. opt-out) as it would not break any existing use case.

Eventually, it could be under the experimental flag (e.g. enabled by default if --experimental) for early testing, and then in 1-2 version enabled by default.

WDYT @olblak ?

@olblak
Copy link
Member Author

olblak commented Nov 4, 2023

I wonder if the pipelineurl shouldn't be true by default (e.g. opt-out) as it would not break any existing use case.

For me pipelineurl set to false means the current behavior which is not showing the pipeline url.
While set to true means we want to see the URL.

In long term I think having this opt-out by default would be better so I don't think the experimental flag is needed here as we don't the current behavior

@dduportal
Copy link
Contributor

I wonder if the pipelineurl shouldn't be true by default (e.g. opt-out) as it would not break any existing use case.

For me pipelineurl set to false means the current behavior which is not showing the pipeline url. While set to true means we want to see the URL.

In long term I think having this opt-out by default would be better so I don't think the experimental flag is needed here as we don't the current behavior

I'm personally in favor of setting it to true by default, e.g. "if you use the new updatecli version, then you will have your PR with the link added by default"

dduportal
dduportal previously approved these changes Nov 4, 2023
Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

Approving, whatever default is chosen

@olblak olblak changed the title chore: show CI url in pullrequest body feat(action): show CI url in pullrequest body Nov 4, 2023
@olblak
Copy link
Member Author

olblak commented Nov 4, 2023

I'm personally in favor of setting it to true by default, e.g. "if you use the new updatecli version, then you will have your PR with the link added by default"

I was taking a more defensive approach, even thought I would also prefer to display the url by default

I am going to change the setting pipelineurl to disablepipelineurl to enable it by default

Signed-off-by: Olblak <me@olblak.com>
@olblak olblak merged commit 6d8e960 into updatecli:main Nov 5, 2023
6 checks passed
Copy link
Contributor

@v1v v1v left a comment

Choose a reason for hiding this comment

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

💯 Thanks so much!!!

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

Successfully merging this pull request may close these issues.

Feature Request: Infer the GitHub action that created the GitHub Pull Request
3 participants