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

fixes Change the application component to agree with kubernetes-sigs application CRD #2154

Merged
merged 51 commits into from
Jan 12, 2019

Conversation

kkasravi
Copy link
Contributor

@kkasravi kkasravi commented Dec 21, 2018

fixes #1856, #1831

  • Each child will have annotations and labels applied that is reference in the Application.Spec.Components
    {
          annotations+: {
            "kubernetes.io/application": params.name,
          },
          labels+: {
            "app.kubernetes.io/name": params.name,
            "app.kubernetes.io/component": resource.metadata.name,
            app: params.name,
            component: resource.metadata.name,
    }
  • Reparenting of the children will be made to point to the controller
  • Deletion of child components will occur if the application is deleted

This change is Reviewable

@kkasravi
Copy link
Contributor Author

@jlewi

This PR is failing at

INFO|2019-01-10T03:12:03|test_jsonnet_formatting.py:51| Running test_jsonnet_formatting
usage: test_jsonnet_formatting.py [-h] [--src_dir SRC_DIR]
                                 [--exclude_dirs EXCLUDE_DIRS]
test_jsonnet_formatting.py: error: unrecognized arguments: --project=kubeflow-ci --artifacts_dir=/mnt/test-data-volume/kubeflow-presubmit-unittests-2154-30d1608-4
974-d87b/output/artifacts
INFO|2019-01-10T03:12:03|test_helper.py:62| Writing file: /mnt/test-data-volume/kubeflow-presubmit-unittests-2154-30d1608-4974-d87b/output/artifacts/junit_test-js
onnet-formatting.xml

Is there a mismatch between kubeflow/testing and kubeflow/kubeflow?

@jlewi
Copy link
Contributor

jlewi commented Jan 10, 2019

I think you need to remove the flag --artifacts_dir

"--artifacts_dir=" + artifactsDir,

We recently changed test_jsonnet_formatting to raise an error on unknown args rather than just ignoring them.

We always checkout HEAD of extra repos like kubeflow/testing so these types of breaks can occur

value: "kubeflow/tf-operator@HEAD;kubeflow/testing@HEAD",

Pinning extra repos to specific versions is one way to prevent those types of breaks.

@kkasravi
Copy link
Contributor Author

/retest

@kkasravi
Copy link
Contributor Author

@jlewi - think this is fairly close to what you had in mind. Separate issue of application CRs and dependencies opened #2257

# builds a set of inputs to be used with envsubst for env.sh
# writeEnv updates env.sh when any env var changes
# eg: envsubst < input.tmpl > env.sh
INPUT+=('PLATFORM=$PLATFORM\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

Much nicer thanks!

@jlewi
Copy link
Contributor

jlewi commented Jan 10, 2019

/lgtm
/approve

I think you just need to fix the test.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jan 10, 2019
@kkasravi
Copy link
Contributor Author

/retest

@kkasravi
Copy link
Contributor Author

@jlewi katib is catching a sync failure in Metacontroller which occasionally happens when the object has been modified during sync processing. Error is harmless.... i should be able to resolve by tomorrow

@kkasravi
Copy link
Contributor Author

kkasravi commented Jan 11, 2019

@jlewi - passing now. What I've observed though in the e2e tests is #980 that is happening in the application-controller.
I've made a few changes which minimizes when this happens by adding resourceVersion to each child I return in the sync hook. Safe to merge.

@jlewi
Copy link
Contributor

jlewi commented Jan 12, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi, kkasravi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit c8f8836 into kubeflow:master Jan 12, 2019
kkasravi added a commit to kkasravi/kubeflow that referenced this pull request Feb 8, 2019
…application CRD (kubeflow#2154)

* snapshot

* snapshot

* snapshot

* snapshot

* snapshot

* mostly working, compositecontroller shouldn't add itself as a childResource

* generate application regardless of emitController value

* snapshot

* better debug info

* debugging why objects are being deleted

* snapshot

* stable

* fmt errors, unit test error

* /retest

* /retest

* /retest

* /retest

* /retest

* update example script

* update example script

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest
@kkasravi kkasravi deleted the application-refactor branch May 13, 2019 14:49
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 11, 2021
…application CRD (kubeflow#2154)

* snapshot

* snapshot

* snapshot

* snapshot

* snapshot

* mostly working, compositecontroller shouldn't add itself as a childResource

* generate application regardless of emitController value

* snapshot

* better debug info

* debugging why objects are being deleted

* snapshot

* stable

* fmt errors, unit test error

* /retest

* /retest

* /retest

* /retest

* /retest

* update example script

* update example script

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest

* /retest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change the application component to agree with kubernetes-sigs application CRD
4 participants