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

Move scm directive to top level #388

Merged
merged 77 commits into from
Dec 15, 2021
Merged

Move scm directive to top level #388

merged 77 commits into from
Dec 15, 2021

Conversation

olblak
Copy link
Member

@olblak olblak commented Nov 21, 2021

Move SCM directive to top-level

Fix #260
Fix #261

Before this pull request, "scm" configurations are configured per resource at the source/condition/target level.
The consequence of that approach is that it generates a lot of duplicates.
Starting from now, each scm configuration is defined once at the top level and each resource (source/condition/target) references the scm using a scm id.

Due to how "scm" is a central component of updatecli, I had to differentiate scm "configuration" provide via a configuration, scm interface used by git and Github to manipulate git repository.
I also split pull request configuration into a top-level one as explained in fix #261 . I was refactoring the scm code anyway

Here is an example of a new configuration
.example

scms:
    updatecliRepository:
        kind: github
        spec: 
            owner: updatecli
            repository: updatecli
           # Skipping some parameter for better example visibility
source:
    updatecliVersion:
        kind: yaml
        scmID: updatecliRepository
        spec: 
            file: example.yaml
            key: image
           # Skipping some parameter for better example visibility

Test

To test this pull request, you can run the following commands:

go build -o bin/updatecli; ./bin/updatecli diff --config examples/updateCli.generic/githubRelease.tpl --values examples/values.yaml
./bin/updatecli show --config examples/updateCli.generic/githubRelease.tpl --values examples/values.yaml

Additional Information

In order to not break the existing updatecli pipeline configuration, I did the extra effort to convert "internally" the old syntax to the new one with the specificity that a GitHub scm would also automatically add pull request configuration.
I am still not sure if I want that to be the definitive approach.

In this pull request, I also decided the move the go packages source, condition, target, under the package pipeline instead of engine as I think it makes more sense to be there, next to scm and pull request.

Changelog

An updatecli pipeline configuration can now use two additional top resources, scms and pullrequests where they respectively allow to configuration sc's configuration and pullrequests' configuration.
Similiar to sources, conditions, and targets: scms and pullrequests expect a "map" of configuration.

The resources of type "source", "condition", "target", "pullrequest" now uses the key scmID to specify an scm dependency.
This avoids duplicating the scm configuration in every resource like it used to be.

Here is an example

scms:
  defaultSCMConfig:
    name: Defines github repository
    kind: github
    spec:
      ...

pullrequests:
  defaultPullrequestConfig:
    name: Defines github pullrequest
    kind: github
    spec:
      ...

sources:
  default:
    name: Get version from a github release
    kind: githubrelease
    sourceID: defaultSCMConfig
    spec:
      ...

SCM

Excepted that the scm configuration is now defined in its top-level configuration scms, parameters remain the same.

Cfr documentation on www.updatecli.io

[cols="1,1,1,4",options=header]
|===
| Name | Required | Default |Description

| name | ✔ |-| Defines the scm name displayed in an updatecli pipeline run
| kind | ✔ | - | Defines the scm resource kind
| spec | ✔ |-| Defines the resource kind configuration

A scm of kind github can't define a configuration "pullrequest" as this is now living in its own place. Cfg the next section.

Pullrequest

[cols="1,1,1,4",options=header]
|===
| Name | Required | Default |Description

| name | ✔ |-| Defines the pull request name displayed in a updatecli pipeline run
| scmID | ✔ |-| Defines the SCM that this pull request depends on
| kind | ✔ | - | Defines the pull request resource kind
| spec | ✔ |-| Defines the resource kind configuration

NOTE: At this stage, the only pull request kind supported is github.

Github

A GitHub pull request can be used to define how to create and configure GitHub pull request.
A pull request will be opened when needed. It also reopens a closed pull request if they are changes between the head branch and the remote one.

Parameters

[cols="1,1,1,4",options=header]
|===
| Name | Required | Default |Description
| description | | - | Defines a custom description to insert at the beginning of a pull request body
| draft | |false| Defines the pull request is in draft mode by default
| labels | |-| Defines a list of GitHub pull request labels. !Must already exist
| maintainercanmodify | | true | Defines if a repository maintainer can modify the pull request
| title | |-| Defines pull request title
|==

Tradeoff

Potential improvement

@olblak olblak marked this pull request as draft November 21, 2021 19:13
@olblak olblak added condition Modify condition resource enhancement New feature or request scm-git SCM of kind "Git" scm-github SCM of type GiHhub target Related to updatecli target labels Nov 21, 2021
@olblak olblak force-pushed the issues/260 branch 2 times, most recently from 6871fe0 to 0f71a70 Compare November 22, 2021 18:52
@olblak
Copy link
Member Author

olblak commented Nov 24, 2021

In the current state of the pullRequest configuration, the pullrequests block expects to duplicate the same configuration as the scm one, like in the following example:

scms:
    repo1:
        kind: github
        spec:
           owner: updatecli
           repository: updatecli
           username: xxx
           token: yyy

targets:
    change1:
       kind: yaml
       scmID: repo1
       spec:
           ....

pullrequests:
    pr1:
        kind: github
        spec:
           owner: updatecli
           repository: updatecli
           username: xxx
           token: yyy
           labels:
                - dependency

Another option (not implemented yet) would be to have some sort of heritage, like:

scms:
    repo1:
        kind: github
        spec:
           owner: updatecli
           repository: updatecli
           username: xxx
           token: yyy

targets:
    change1:
        kind: yaml
        sourceID: repo1
        spec:
            ....

pullrequests:
    pr1:
        kind: github
        dependTargetsOn:
            - change1
        spec:
           labels:
                - dependency

In this second example, we retrieve the scm spec via "pullequests.pr1.dependsTargetsOn[0].sourceID" and then merge it with pullRequests.pr1.spec
If we go down that path then the pullrequest kind must be exactly the same as the scm kind which is GitHub in this case.

@olblak
Copy link
Member Author

olblak commented Nov 24, 2021

A second open question that I have is how to reference that part of the code in this pullRequest will be remove once we fully remove the deprecated to specify scm configuration.
Should I?
. Put a link to this pullRequest
. Create a new issue mentioning that some cleanup is needed and put that link in the issue
. Identify which releases version the current PR will land and put a link to the version

@olblak
Copy link
Member Author

olblak commented Nov 24, 2021

@dduportal I propose to target the milestone v0.17.0 with only this PR

@olblak olblak modified the milestone: 0.17.0 Nov 24, 2021
@dduportal dduportal added this to the 0.17.0 milestone Nov 24, 2021
@olblak olblak removed this from the 0.17.0 milestone Nov 24, 2021
@olblak olblak marked this pull request as ready for review November 24, 2021 15:14
olblak and others added 14 commits November 25, 2021 20:29
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>
Rename scm interface to scmHandler

Move scm package under pipeline
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
olblak and others added 4 commits December 10, 2021 09:50
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
pkg/core/pipeline/pullRequest/main.go Outdated Show resolved Hide resolved
pkg/core/pipeline/pullRequest/main.go Outdated Show resolved Hide resolved
pkg/core/pipeline/pullRequests.go Outdated Show resolved Hide resolved
pkg/core/pipeline/condition/main.go Show resolved Hide resolved
olblak and others added 5 commits December 10, 2021 14:12
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>
Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Dec 10, 2021

I identified another regression where files defined modified by a target is done locally instead of the temporary location.

bin/updatecli diff --config /Projects/Jenkins-infra/pipeline-library/updatecli/updatecli.d --values /Projects/Jenkins-infra/pipeline-library/updatecli/values.yaml --debug would fail

@dduportal
Copy link
Contributor

Just tried again the dduportal/pipeline-library example with the YAML manifest from previous comment with the commit 628a98a and still the same error when applying (pull Request stage:\t\"Head can't be blank\").

I would be interested to pair with because I couldn't reproduce your error

As seen together, it works as expected: I wasn't using the correct binary. Sorry for the PEBKAC.

@olblak
Copy link
Member Author

olblak commented Dec 10, 2021

I identified another regression where files defined modified by a target is done locally instead of the temporary location.

bin/updatecli diff --config /Projects/Jenkins-infra/pipeline-library/updatecli/updatecli.d --values /Projects/Jenkins-infra/pipeline-library/updatecli/values.yaml --debug would fail

@dduportal

The root cause of the error is coming from #401 and is outside the scope of the current pull request.
That pull request introduce a test to check that a specific file exist, which is really great but it doesn't consider the updatecli temporary directory during the validation.

So at the moment, we need to execute updatecli at the root of the repository to have a successful run.

@dduportal
Copy link
Contributor

I identified another regression where files defined modified by a target is done locally instead of the temporary location.
bin/updatecli diff --config /Projects/Jenkins-infra/pipeline-library/updatecli/updatecli.d --values /Projects/Jenkins-infra/pipeline-library/updatecli/values.yaml --debug would fail

@dduportal

The root cause of the error is coming from #401 and is outside the scope of the current pull request. That pull request introduce a test to check that a specific file exist, which is really great but it doesn't consider the updatecli temporary directory during the validation.

So at the moment, we need to execute updatecli at the root of the repository to have a successful run.

Hop, opened an issue to track this #411 \o/ Good catch!

@dduportal
Copy link
Contributor

First batch of testings:

@dduportal
Copy link
Contributor

dduportal commented Dec 13, 2021

First batch of testings:

* https://github.com/dduportal/pipeline-library/tree/chore/updatecli-0.17
  
  * ✅ The `diff` works as expected with the nice message in the report section "pull request" with no change detected
  * ✅ The `apply` works as expected (e.g. no error and no PR opened because no changes)

* https://github.com/dduportal/charts/tree/chore/updatecli-0.17 (only the `updatecli/updatecli.d/charts/*yaml` manifests)
  
  * ✅ `diff` works as expected and detects 6 changes (over the 32 pipelines)
  * ❌ `apply` doesn't work as expected: it applies the changes locally instead of openeing the pull requests. Can you check if I did something wrong @olblak ?

As per https://github.com/updatecli/updatecli/pull/388/files#diff-d217d87f213d484dd97f5b4a68a98a57de54d6404c000f7c81167a0371cb963dR35, I was missing the scmID attribute in the target examples. Once added, it works as expected (it created https://github.com/dduportal/charts/pull/11) ✅

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.

🚀 👏

@olblak
Copy link
Member Author

olblak commented Dec 13, 2021

rocket clap

Awesome, thanks for you helping on testing this refactoring. Once the documentation is written, I'll merge this PR and trigger release

@olblak olblak merged commit 7ab1dad into updatecli:main Dec 15, 2021
@olblak olblak deleted the issues/260 branch December 15, 2021 08:23
@olblak
Copy link
Member Author

olblak commented Dec 15, 2021

Once again, thanks for your help on this refactoring

dduportal added a commit to dduportal/website that referenced this pull request Mar 20, 2022
- Deprecated field "version" - updatecli/updatecli#590
- Deprecated fields "postfix/prefix" - updatecli/updatecli#590
- Deprecated fields "replacers" - updatecli/updatecli#592
- SCM setup moved to the top level directive scms - updatecli/updatecli#388
- Deprecated top-level field source (singular source) - updatecli/updatecli#589

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
dduportal added a commit to updatecli/website that referenced this pull request Mar 22, 2022
- Deprecated field "version" - updatecli/updatecli#590
- Deprecated fields "postfix/prefix" - updatecli/updatecli#590
- Deprecated fields "replacers" - updatecli/updatecli#592
- SCM setup moved to the top level directive scms - updatecli/updatecli#388
- Deprecated top-level field source (singular source) - updatecli/updatecli#589

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
condition Modify condition resource enhancement New feature or request scm-git SCM of kind "Git" scm-github SCM of type GiHhub target Related to updatecli target
Projects
None yet
3 participants