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: adds abilty to create build pipeline job #102

Closed

Conversation

FredrikMWold
Copy link
Contributor

This pull request adds the build command to create pipeline-job.
The logic/plumbing to create a build pipeline was already present. I just added the cobra command itself.

@FredrikMWold FredrikMWold force-pushed the feat/add_build_pipeline branch 2 times, most recently from 5c134e9 to 1d2024b Compare July 26, 2024 14:10
@FredrikMWold
Copy link
Contributor Author

bump

Copy link
Contributor

@satr satr left a comment

Choose a reason for hiding this comment

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

Suggested changes

cmd/createBuildPipelineJob.go Show resolved Hide resolved
Comment on lines +37 to +40
if appName == nil || *appName == "" {
return errors.New("application name is required")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if appName == nil || *appName == "" {
return errors.New("application name is required")
}
if appName == nil || *appName == "" || branch == "" {
return errors.New("application name and branch are required")
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Branch is mandatory here

Copy link
Contributor

Choose a reason for hiding this comment

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

Or add following

	if err := createBuildPipelineJobCmd.MarkFlagRequired(flagnames.Application); err != nil {
		log.Fatalf("Error during command initialization: %v", err)
	}

instead of

   if appName == nil || *appName == "" {
			return errors.New("application name is required")
		}

to be consistent

}

jobName := newJob.GetPayload().Name
log.SetOutput(cmd.OutOrStdout())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.SetOutput(cmd.OutOrStdout())

Copy link
Contributor

Choose a reason for hiding this comment

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

Log output has to be uniformal with other commands

Copy link
Contributor

Choose a reason for hiding this comment

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

Radix CLI prints to stdout only objects , e.g.

fmt.Fprintf(os.Stdout, "%s", b)

messages it prints to stderr

Copy link
Contributor

Choose a reason for hiding this comment

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

These requested changes are to keep the product and its code consistent. The PR unfortunately cannot be approved without these changes.

@FredrikMWold FredrikMWold force-pushed the feat/add_build_pipeline branch 2 times, most recently from 53e0950 to af7d5e0 Compare August 26, 2024 11:46
@FredrikMWold FredrikMWold requested a review from satr August 26, 2024 11:50
@satr
Copy link
Contributor

satr commented Oct 22, 2024

This code is used (with applied requested changes) to this version](https://github.com/equinor/radix-cli/releases/tag/v1.23.0)

@satr satr closed this Oct 22, 2024
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.

2 participants