Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add build-tool cli with a crd validation command #3

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bnsblue
Copy link
Contributor

@bnsblue bnsblue commented Sep 9, 2019

This PR adds support of crd validation as a command in a new CLI in flytepropeller

@bnsblue
Copy link
Contributor Author

bnsblue commented Sep 9, 2019

PTAL @EngHabu @kumare3

Gopkg.toml Outdated
@@ -85,6 +86,10 @@ required = [
# revision = "6702109cc68eb6fe6350b83e14407c8d7309fd1a"
version = "kubernetes-1.13.5"

[[override]]
name = "k8s.io/kube-openapi"
revision = "743ec37842bffe49dd4221d9026f30fb1d5adbc4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do they not have versions?

Gopkg.toml Outdated

[[constraint]]
name = "github.com/kubeflow/crd-validation"
source = "github.com/bnsblue/crd-validation.git"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try to get this forked under lyft... if that proved to be too hard, then this is ok we can switch it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I am waiting on the fork request to be approved...

cmd/build-tool/cmd/crd/flyteworkflow.go Outdated Show resolved Hide resolved
cmd/build-tool/cmd/crd_validation.go Show resolved Hide resolved
return err
}

compilerErrors.SetIncludeSource()
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

err := c.initConfig()
var generator *crd.FlyteWorkflowGenerator
if err != nil {
log.Println("Output will be written to Stdout")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason you don't just fail here instead of trying to "recover" ?

cmd/build-tool/main.go Outdated Show resolved Hide resolved
fmt.Println(err)
os.Exit(1)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
}
}

cmd/kubectl-flyte/cmd/root.go Outdated Show resolved Hide resolved
bnsblue and others added 3 commits September 25, 2019 15:30
Add a new line at the end of the file

Co-Authored-By: Haytham AbuelFutuh <[email protected]>
@kumare3
Copy link
Contributor

kumare3 commented Mar 7, 2020

ping @bnsblue, do we want to move ahead with this?

@bnsblue
Copy link
Contributor Author

bnsblue commented Mar 8, 2020

@kumare3 Yes we do. I'll take some time to merge this during the next week

kumare3 pushed a commit to nuclyde-io/flytepropeller that referenced this pull request Feb 4, 2021
* add namepace configuration

* add namespace files

* remove unused var

* update propeller executor config

* use propeller executor configuration

* to lowercase

* fix conflicts

* update namespace test

* remove namespaceMapping variable

* fix compile issues

* add project-domain option and remove incorrect log messages

* upd mock configuration provider

* update unit tests

* Merge logs on task execution event updates (flyteorg#18)

* use fallthrough

* Correct Lint Errors and Add Documentation on Pre-Commit (flyteorg#19)

* README update

* Fix lint errors

* add namepace configuration

* add namespace files

* remove unused var

* update propeller executor config

* use propeller executor configuration

* to lowercase

* fix conflicts

* update namespace test

* remove namespaceMapping variable

* fix compile issues

* add project-domain option and remove incorrect log messages

* upd mock configuration provider

* update unit tests

* use fallthrough

* fix conflicts

* fix more conflicts

* lint

* remove duplicate package

* fix lint errors
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.

3 participants