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

Add gcp secret parameter to container op #261

Merged
merged 6 commits into from
Nov 15, 2018
Merged

Add gcp secret parameter to container op #261

merged 6 commits into from
Nov 15, 2018

Conversation

IronPan
Copy link
Member

@IronPan IronPan commented Nov 14, 2018

This would allow specifying gcp secret to the container, for accessing all GCP API.


This change is Reviewable

@IronPan
Copy link
Member Author

IronPan commented Nov 14, 2018

/assign @gaoning777 @Ark-kun

@coveralls
Copy link

coveralls commented Nov 14, 2018

Pull Request Test Coverage Report for Build 363

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 7 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+10.6%) to 78.152%

Files with Coverage Reduction New Missed Lines %
src/lib/Utils.ts 1 90.16%
src/pages/NewRun.tsx 1 96.37%
src/pages/PipelineSelector.tsx 5 56.1%
Totals Coverage Status
Change from base Build 321: 10.6%
Covered Lines: 1775
Relevant Lines: 2193

💛 - Coveralls

@@ -144,6 +144,17 @@ def _op_to_template(self, op):
if op.cpu_request:
template['container']['resources']['requests']['cpu'] = op.cpu_request

if op.gcp_secret:
template['container']['env'] = {}
Copy link
Contributor

@Ark-kun Ark-kun Nov 14, 2018

Choose a reason for hiding this comment

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

  template['container']['env'] = [
    {
      'name': 'GOOGLE_APPLICATION_CREDENTIALS',
      'value': '/secret/gcp-credentials/user-gcp-sa.json',
    },
  ]
  template['container']['volumeMounts'] = [
    {
      'name': 'gcp-credentials',
      'mountPath': '/secret/gcp-credentials',
    },
  ]
  template['volumes'] = [
    {
      'name': 'gcp-credentials',
      'secret': {
        'secretName': op.gcp_secret,
      }
    },
  ]

golden_output = {
'container': {
'image': 'image',
'args': [
'echo {{inputs.parameters.msg1}} {{inputs.parameters.msg2}} | tee /tmp/message.txt'
],
'command': ['sh', '-c'],
'env': {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test running this pipeline?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please create the secret in your cluster, compile a pipeline, submit it to the cluster and check for errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with Alexey for testing it manually before merging

@Ark-kun
Copy link
Contributor

Ark-kun commented Nov 14, 2018

It would be great to have an integration test for this in future.

Copy link
Contributor

@gaoning777 gaoning777 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaoning777

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

1 similar comment
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaoning777

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

@Ark-kun
Copy link
Contributor

Ark-kun commented Nov 15, 2018

/approve cancel

Copy link
Contributor

@Ark-kun Ark-kun left a comment

Choose a reason for hiding this comment

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

Waiting for the test results.

@Ark-kun
Copy link
Contributor

Ark-kun commented Nov 15, 2018

Added issue about per-container volumes to Argo argoproj/argo-workflows#1094

@Ark-kun
Copy link
Contributor

Ark-kun commented Nov 15, 2018

/lgtm

@IronPan
Copy link
Member Author

IronPan commented Nov 15, 2018

/test presubmit-e2e-test

@k8s-ci-robot k8s-ci-robot merged commit 7e34b12 into master Nov 15, 2018
@IronPan IronPan deleted the yangpa/foo branch December 10, 2018 22:26
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
…w#261)

* Add a python function to GC old Argo workflows and cloud endpoints

kubeflow/testing#53 GC old Argo Workflows
kubeflow/testing#87 cron job to GC old resources.
kubeflow/testing#268 Maximum number of services reached.

* Fix lint.

* Revert files that shouldn't be checked in.

* Fix loop termination criterion.
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
* Add support for response in OpenAPI

* Add unit tests for responses

* Update E2E tests for responses

* Cleanup

* Add panic for unexpected error

* Encapsulate req + resp schemas

* Update tests for new Schema params and templatize

* Rm newline
HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024
…kubeflow#261)

* update scheduledworkflow image to run with pipelinerun

* update complete label to based on PersistedFinalState
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.

5 participants