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

LOG-1700: Fix ES-Proxy master to version 1.0 #87

Merged
merged 1 commit into from Aug 17, 2021

Conversation

vimalk78
Copy link
Contributor

Description

Parent Task: Make operand images reusable across openshift-logging releases
This PR fixes the master branch of ES_Proxy to version "v1.0". Once this is merged, CPaaS Dockerfile wil be changes to reflext the same version.

/cc
/assign

/cherry-pick

Links

@vimalk78
Copy link
Contributor Author

/hold

@vimalk78
Copy link
Contributor Author

/assign @igor-karpukhin
/cc @jcantrill

@openshift-ci openshift-ci bot requested a review from jcantrill August 17, 2021 12:21
@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2021
Makefile Outdated
@@ -47,7 +47,15 @@ vendor:
.PHONY: vendor

image:
podman build -f Dockerfile -t $(LOCAL_IMAGE_TAG) .
CI_CONTAINER_VERSION="v1.0" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we call this target other then as a developer? CI has it's own config which calls make build. That does not seem to trace to this target. What will CPaaS do? Does this need to be moved to maybe the Dockerfile.in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI does not use make build
CI uses specified Dockerfile path https://github.com/openshift/release/blob/master/ci-operator/config/openshift/elasticsearch-proxy/openshift-elasticsearch-proxy-release-5.0.yaml#L63-L70

perhaps this change should go into hack/generate-dockerfile-from-midstream , we should be able to set all variables except commit_id. does CI has a way to capture it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved the change to Dockerfile generation

Copy link
Contributor

Choose a reason for hiding this comment

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

CI does not use make build

CI uses make build via the Dockerfile https://github.com/openshift/elasticsearch-proxy/blob/master/Dockerfile.in#L19 which will call the default target https://github.com/openshift/elasticsearch-proxy/blob/master/Makefile#L30

@vimalk78 vimalk78 force-pushed the es-proxy-v1.0 branch 4 times, most recently from 42af31a to 7b1e110 Compare August 17, 2021 13:52
Makefile Outdated
@@ -2,6 +2,11 @@ export GOROOT=$(shell go env GOROOT)
export GOFLAGS=-mod=vendor
export GO111MODULE=on

ES_PROXY_MAJOR="1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason not to add these directly to the Dockerfile.in? There is nothing about the changes where we are calculating version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

  Parent Task: https://issues.redhat.com/browse/LOG-1635 Make operand images reusable across openshift-logging releases
Copy link
Contributor

@jcantrill jcantrill left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 17, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill, vimalk78

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2021
@vimalk78
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2021
@openshift-ci openshift-ci bot merged commit 0c1dea6 into openshift:master Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants