Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

Adds --distribution flag, refactor k8s generation #250

Merged
merged 1 commit into from
Sep 13, 2017

Conversation

cdrage
Copy link
Collaborator

@cdrage cdrage commented Sep 6, 2017

This commit adds the --distribution flag as well as refactors the
generate.go file to one single file, kubernetes.go

This allows for all future changes to simply be made within
kubernetes.go rather than two separate files with similar code
(generate.go + kubernetes.go)

@cdrage
Copy link
Collaborator Author

cdrage commented Sep 6, 2017

This refactors the kubernetes.go file as well so future implementations such as adding openshift.go is straight-forward.

func ExecuteKubectl(paths []string, args ...string) error {
// Generate Kubernetes Artifacts and either writes to file
// or uses kubectl to deploy.
func CreateKubernetesArtifacts(paths []string, generate bool, args ...string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@cdrage I like the idea, but now this function is doing too many things. How about we create a new function which either writes to a file or deploys to cluster and let this one be?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's how it was before and both functions used / copied the same code hence why I've gone ahead and made it into one function that either writes or executes / deploy.

I agree it may be pushing it in regards to doing too many things, but before it was to similar functions in two separate files.

Copy link
Collaborator

@concaf concaf Sep 13, 2017

Choose a reason for hiding this comment

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

What I meant was that, in the current master, the following part is common for both Generate() and ExecuteKubectl() functions, so we can abstract that out in a single function and reuse it in all the cmd/* files. And the remaining uncommon part can go into separate functions, one with writeObject part, another with runKubectl part.

So -

  • Function 1 with common part
	files, err := GetAllYAMLFiles(paths)
	if err != nil {
		return errors.Wrap(err, "unable to get YAML files")
	}

	inputs, err := getApplicationsFromFiles(files)
	if err != nil {
		return errors.Wrap(err, "unable to get kedge definitions from input files")
	}

	for _, input := range inputs {

		ros, extraResources, err := spec.CoreOperations(input.data)
		if err != nil {
			return errors.Wrap(err, "unable to perform controller operations")
		}

		// write all the kubernetes objects that were generated
		for _, runtimeObject := range ros {

			data, err := yaml.Marshal(runtimeObject)
			if err != nil {
				return errors.Wrap(err, "failed to marshal object")
			}
  • Function 2 with writeObject part
			err = writeObject(data)
			if err != nil {
				return errors.Wrap(err, "failed to write object")
			}
		}

		for _, file := range extraResources {
			// change the file name to absolute file name
			// then read the file and then pass it to writeObject
			file = findAbsPath(input.fileName, file)
			data, err := ioutil.ReadFile(file)
			if err != nil {
				return errors.Wrap(err, "file reading failed")
			}
			err = writeObject(data)
			if err != nil {
				return errors.Wrap(err, "failed to write object")
			}
		}
	}
  • Function 3 with runKubectl part
			arguments := append(args, "-f", "-")
			err = runKubectl(arguments, data)
			if err != nil {
				return errors.Wrap(err, "kubectl error")
			}
		}

		for _, file := range extraResources {
			// change the file name to absolute file name
			file = findAbsPath(input.fileName, file)

			// We need to add "-f absolute-filename" at the end of the command passed to us to
			// pass the generated files.
			// e.g. If the command and arguments are "apply --namespace staging", then the
			// final command becomes "kubectl apply --namespace staging -f absolute-filename"
			arguments := append(args, "-f", file)
			err = runKubectl(arguments, nil)
			if err != nil {
				return errors.Wrap(err, "kubectl error")
			}
		}

So instead of a bigger one, we break it down to smaller 3 more testable functions.
WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@containscafeine I initially separated it into three different functions but found that I was over-engineering it. For example, I put this into one function:

	files, err := GetAllYAMLFiles(paths)
	if err != nil {
		return errors.Wrap(err, "unable to get YAML files")
	}

	inputs, err := getApplicationsFromFiles(files)
	if err != nil {
		return errors.Wrap(err, "unable to get kedge definitions from input files")
	}

Which was literally a function calling two other functions and erroring it out.. Only reducing the amount of lines by 4.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to have two functions, one that generates Kubernetes artifacts (returns string that is complete representation of kedge file including extra resources or error). And other that takes that string and calls kubectl.

Having one big function with a boolean argument that switches what that function is doing is a bit untransparent.
There are two different functionalities in that function hidden behind one boolean switch.

I would do it differently, but I'm OK with how it is done in this PR.

@cdrage
Copy link
Collaborator Author

cdrage commented Sep 8, 2017

Turning into a larger PR, but I've gone ahead and added some small tests too.

@cdrage
Copy link
Collaborator Author

cdrage commented Sep 11, 2017

Hey all, since #237 needs to be implemented too, I'm going to change this back to a WIP, update the commit message as well as add the "deploymentconfig" option / controller.

@cdrage cdrage changed the title Adds --distribution flag, refactors generating Kubernetes artifacts Adds --distribution flag, refactor k8s generation, add deploymentconfig Sep 11, 2017
@cdrage cdrage force-pushed the add-distribution-flag branch 2 times, most recently from 646b4ea to 395ab50 Compare September 11, 2017 19:36
@cdrage cdrage changed the title Adds --distribution flag, refactor k8s generation, add deploymentconfig [WIP] Adds --distribution flag, refactor k8s generation, add deploymentconfig Sep 11, 2017
@kadel
Copy link
Member

kadel commented Sep 12, 2017

Hey all, since #237 needs to be implemented too, I'm going to change this back to a WIP, update the commit message as well as add the "deploymentconfig" option / controller.

Wouldn't it be better to address #237 in separate PR?

If you put both in one, it might end up being quite big PR

@cdrage
Copy link
Collaborator Author

cdrage commented Sep 12, 2017

Okay, no worries! Want to do a review of this current PR @kadel and I can implement #237 in a separate PR?

@cdrage cdrage force-pushed the add-distribution-flag branch 2 times, most recently from 6357b49 to 5e22a43 Compare September 12, 2017 14:36
@cdrage cdrage changed the title [WIP] Adds --distribution flag, refactor k8s generation, add deploymentconfig Adds --distribution flag, refactor k8s generation, add deploymentconfig Sep 12, 2017
@cdrage cdrage changed the title Adds --distribution flag, refactor k8s generation, add deploymentconfig Adds --distribution flag, refactor k8s generation Sep 12, 2017
@cdrage
Copy link
Collaborator Author

cdrage commented Sep 12, 2017

Review time 👍

@cdrage
Copy link
Collaborator Author

cdrage commented Sep 13, 2017

@containscafeine Do you mind if if we continue with this PR and then we can refactor it later, in order for me to open up and continue with #257 and your jobs PR #116 ?

Reason for combing is to get rid of generate.go so we eventually have kubernetes.go and openshift.go

I've gone ahead and added a note to the code as well as TODO 👍

This commit adds the --distribution flag as well as refactors the
generate.go file to one single file, kubernetes.go.

This allows for all future changes to simply be made within
kubernetes.go rather than two separate files with similar code
(generate.go + kubernetes.go)
Copy link
Collaborator

@concaf concaf left a comment

Choose a reason for hiding this comment

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

Yay, pragmatism FTW! Let's do the refactor later.

@concaf concaf merged commit 7600dcc into kedgeproject:master Sep 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants