-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
MetadataStore: Update to release metadata-envoy in each release #4026
MetadataStore: Update to release metadata-envoy in each release #4026
Conversation
not sure whether it will be updated in another PR and test |
/lgtm not sure whether it will be updated in another PR and test /hold |
In addition to what @rmgogogo provided, you also need to build the image for each presubmit test in https://github.com/kubeflow/pipelines/blob/master/test/cloudbuild/batch_build.yaml#L40 You need these changes in the same PR to pass e2e tests |
59ff06c
to
de90c24
Compare
I updated the pointed files. Looks like its still is not sufficient. |
.release.cloudbuild.yaml
Outdated
waitFor: ['pullMetadataEnvoy'] | ||
- id: 'tagMetadataEnvoyCommitSHA' | ||
name: 'gcr.io/cloud-builders/docker' | ||
args: ['tag', 'gcr.io/$PROJECT_ID/metadata-writer:$COMMIT_SHA', 'gcr.io/ml-pipeline/metadataenvoy:$COMMIT_SHA'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a typo?
metadata-writer -> metadataenvoy
de90c24
to
0e8cbe1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dushyanthsc!
There's one minor issue
.release.cloudbuild.yaml
Outdated
@@ -398,8 +406,8 @@ steps: | |||
- | | |||
docker tag gcr.io/$PROJECT_ID/metadata-envoy:$COMMIT_SHA gcr.io/ml-pipeline/google/pipelines/metadataenvoy:$(cat /workspace/mm.ver) | |||
docker tag gcr.io/$PROJECT_ID/metadata-envoy:$COMMIT_SHA gcr.io/ml-pipeline/google/pipelines-test/metadataenvoy:$(cat /workspace/mm.ver) | |||
docker push gcr.io/ml-pipeline/google/pipelines/metadataenvoy:$(cat /workspace/mm.ver) | |||
docker push gcr.io/ml-pipeline/google/pipelines-test/metadataenvoy:$(cat /workspace/mm.ver) | |||
docker push gcr.io/ml-pipeline/google/pipelines/metadata-envoy:$(cat /workspace/mm.ver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I might have not been clear enough.
For standalone deployment images, we should use -
separated.
For marketplace images, there cannot be -
s in them.
These are below /google/ directory, so they are marketplace images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I think it my mistake, I didn't read the path correctly. I have fixed it now.
0e8cbe1
to
1e07313
Compare
Thanks @dushyanthsc! note the entire test infra is failing, we might want to retry these tests tomorrow to see if they recover |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy, rmgogogo 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 |
/unhold |
/retest |
/retest |
@Bobgy Can you remove the "do-not-merge/hold label" if the changes are good. Thanks |
Sorry, that was a mistake.... github UI has concurrent conflicts when doing two actions at the same time. |
Metadata envoy image was built in every build but only release for marketplace. This change releases metadata-envoy image for OSS also in each release.