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

Create release command #145

Merged
merged 13 commits into from
Aug 5, 2024
Merged

Conversation

nicholasSUSE
Copy link
Collaborator

@nicholasSUSE nicholasSUSE commented Jul 30, 2024

This Pull Request is for replacing: forward-port script

With Golang safe code that can be used within ecm-distro-tools

Issue: rancher/ecm-distro-tools#446

@nicholasSUSE nicholasSUSE requested a review from a team as a code owner July 30, 2024 21:42
Copy link
Contributor

@rafaelbreno rafaelbreno left a comment

Choose a reason for hiding this comment

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

Just a couple of comments, besides that LGTM!

Approved as they are non-blockers.

pkg/git/git.go Outdated
Comment on lines 289 to 292
if err := cmd.Run(); err != nil {
return err // Return the error if the file does not exist or any other git error occurs
}
return nil // Return nil if the file exists
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can instead directly return the cmd.Run() call.

Suggested change
if err := cmd.Run(); err != nil {
return err // Return the error if the file does not exist or any other git error occurs
}
return nil // Return nil if the file exists
return exec.Command("git", "-C", g.Dir, "cat-file", "-e", target).Run()

pkg/git/git.go Outdated
Comment on lines 299 to 302
if err := cmd.Run(); err != nil {
return err
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Suggested change
if err := cmd.Run(); err != nil {
return err
}
return nil
return exec.Command("git", "-C", g.Dir, "reset", "HEAD").Run()

}

// ReleaseVersions represents the versions within the release.yaml file
type ReleaseVersions map[string][]string

Choose a reason for hiding this comment

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

I don' think this needs to be a named type and it doesn't need to be exported.

)

// Release holds necessary metadata to release a chart version
type Release struct {

Choose a reason for hiding this comment

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

Is this used outside of this package? If not, please unexport it.

Copy link
Collaborator Author

@nicholasSUSE nicholasSUSE Aug 2, 2024

Choose a reason for hiding this comment

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

It is not used outside yet, except from main.go file.

But this will be used by auto-forward-port in a future PR.

return nil
}

func checkAssetReleased(chartVersion string) bool {

Choose a reason for hiding this comment

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

Let's change this from bool to error. We shouldn't let errors get ignored.

// example: assets/longhorn/longhorn-100.0.0+up0.0.0.tgz
assetTgz = chart + "-" + version + ".tgz"
assetPath = "assets/" + chart + "/" + assetTgz
return

Choose a reason for hiding this comment

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

Suggested change
return

return err
}

if err := r.git.ResetHEAD(); err != nil {

Choose a reason for hiding this comment

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

Suggested change
if err := r.git.ResetHEAD(); err != nil {
return r.git.ResetHEAD(); err != nil {

updatedYAMLIndented := enforceYamlStandard(updatedYAML)

// Write the updated YAML back to the file
if err := os.WriteFile(r.ReleaseYamlPath, updatedYAMLIndented, 0644); err != nil {

Choose a reason for hiding this comment

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

Suggested change
if err := os.WriteFile(r.ReleaseYamlPath, updatedYAMLIndented, 0644); err != nil {
return os.WriteFile(r.ReleaseYamlPath, updatedYAMLIndented, 0644); err != nil {

func (r *Release) UpdateReleaseYaml() error {
var releaseVersions ReleaseVersions

rvBytes, err := os.ReadFile(r.ReleaseYamlPath)

Choose a reason for hiding this comment

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

Rather than reading directly into memory, we should use the stream interface methods of encoder and decoder and we can condense the operations down as well as read the stream of bytes. This goes for the unmarshal function below.

}

// enforceYamlStandard adds indentation to list items in a YAML string and a new line at the end of the file.
func enforceYamlStandard(yamlBytes []byte) []byte {

Choose a reason for hiding this comment

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

Is there no library for formatting or validating?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was removed.
I am using "gopkg.in/yaml.v3" now.

pkg/git/git.go Outdated
targetBranch := upstreamRemote + "/" + branch

cmd := exec.Command("git", "-C", g.Dir, "checkout", targetBranch, "--", file)
if err := cmd.Run(); err != nil {

Choose a reason for hiding this comment

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

Suggested change
if err := cmd.Run(); err != nil {
return cmd.Run(); err != nil {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactored all functions here

@@ -22,7 +22,7 @@ type Asset struct {
// Dependencies holds the necessary filesystem,
// assets versions map, version rules and methods to apply the lifecycle rules in the target branch
type Dependencies struct {
rootFs billy.Filesystem
RootFs billy.Filesystem

Choose a reason for hiding this comment

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

Is this field accessed outside of this package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is used by auto package

@nicholasSUSE nicholasSUSE merged commit 3352c7e into rancher:master Aug 5, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants