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

Refactor target resource to return more metadata #1296

Merged
merged 16 commits into from
Apr 23, 2023

Conversation

olblak
Copy link
Member

@olblak olblak commented Apr 21, 2023

In an attempt to generate better report out of Updatecli execution, I am refactoring target resources with following compopents:

  • Merge the functions Target() and TargetFromSCM

As the Updatecli project evolved, I realize that those two functions are useless code duplication, and introduce more complexity. Therefor I went over all plugin to simplify those two functions into one named Target()

The risk associated with this refactoring is that a target plugin working with scm repositories can be broken and this hard to fully tests in dev environment.

  • Target function now return additional metadata during execution

I modify both the target function parameters and returned values. Target uses the new struct result.Target to store\

  • Old value
  • New Value
  • execution result (success, attention, failure, skipped)
  • message such as update yaml key "version" from x to y in file z

In very short

-       Target(source string, dryRun bool) (bool, error)
-       TargetFromSCM(source string, scm scm.ScmHandler, dryRun bool) (changed bool, files []string, message string, err error)
+       Target(source string, scm scm.ScmHandler, dryRun bool, targetResult *result.Target) (err error)

Where result.Target is:

// Target holds target execution result
type Target struct {
    // Name holds the target name
    Name string
    /*  
        Result holds the target result, accepted values must be one:
            * "SUCCESS"
            * "FAILURE"
            * "ATTENTION"
            * "SKIPPED"
    */
    Result string
    // OldInformation stores the old information detected by the target execution
    OldInformation string
    // NewInformation stores the new information updated by during the target execution
    NewInformation string
    // Description stores the target execution description
    Description string
    // Files holds the list of files modified by a target execution
    Files   []string
    Changed bool
}

Test

Considering that this pullrequest modifies all target plugin, we need as many tests as possible

Additional Information

Tradeoff

Not every target plugin has the ability to retrieve the old value before updating.
For example the shell plugin allows to run whatever scripts needed. For those situation, I decided to use the default value "unkown"

Potential improvement

We need similar refactoring for the condition and source plugins but I prefer to do as stage at the time considering the amount of files changed.

olblak and others added 5 commits April 19, 2023 15:01
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 Apr 21, 2023

An example of possible report

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

REPORTS:


✔ Bump venom version:
	Source:
		✔ [latestVersion] Get latest Venom release (kind: githubrelease)
	Target:
		✔ [goWorkflow] Bump Venom version to v1.1.0
			=> all contents from 'file' and 'files' combined already up to date


Run Summary
===========
Pipeline(s) run:
  * Changed:	0
  * Failed:	0
  * Skipped:	0
  * Succeeded:	1
  * Total:	1

or

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

REPORTS:


✔ Bump epinio server version:
	Source:
		✔ [epinio] Get Latest epinio version (kind: gittag)
	Condition:
		✔ [dockerImage] Check that ghcr.io/epinio/epinio-server:v1.6.1 is published (kind: dockerimage)
		✔ [dockerImageRegistry] Check that 'ghcr.io' registry is used in Helm chart epinio (kind: yaml)
		✔ [dockerImageRepository] Check that container image repository 'epinio/epinio-server' is used in Helm chart epinio (kind: yaml)
	Target:
		✔ [epinioui] Update epinio version in chart epinio
			=> key "epinio-ui.epinioVersion" already set to "v1.6.1", from file "/tmp/updatecli/github/olblak/epinio-helm-chart/chart/epinio/values.yaml", 
		✔ [helm-charts] Update epinio version in chart epinio
			=> key "appVersion" already set to "v1.6.1", from file "/tmp/updatecli/github/olblak/epinio-helm-chart/chart/epinio/Chart.yaml", 


Run Summary
===========
Pipeline(s) run:
  * Changed:	0
  * Failed:	0
  * Skipped:	0
  * Succeeded:	1
  * Total:	1

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>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak olblak merged commit 2b66d08 into updatecli:main Apr 23, 2023
5 checks passed
@olblak olblak added the cleanup label May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant