chore(#199): migrate from devshift registry to quay.io by hemanik · Pull Request #218 · arquillian/ike-prow-plugins

hemanik · 2018-07-02T08:50:50Z

The plugins and hook services are migrated to the new Quay registry - specifically https://quay.io/organization/openshiftio.

The new repositories for the same are as follows:

CentOS containers:
https://quay.io/openshiftio/ike-prow-plugins-hook
https://quay.io/openshiftio/ike-prow-plugins-test-keeper
https://quay.io/openshiftio/ike-prow-plugins-pr-sanitizer
https://quay.io/openshiftio/ike-prow-plugins-work-in-progress

RHEL containers:
https://quay.io/openshiftio/rhel-ike-prow-plugins-hook
https://quay.io/openshiftio/rhel-ike-prow-plugins-test-keeper
https://quay.io/openshiftio/rhel-ike-prow-plugins-pr-sanitizer
https://quay.io/openshiftio/rhel-ike-prow-plugins-work-in-progress

Fixes: #199

centos-ci · 2018-07-02T08:50:52Z

Can one of the admins verify this patch?

bartoszmajsak · 2018-07-02T08:51:52Z

[test]

bartoszmajsak · 2018-07-02T08:53:05Z

-	$(DOCKER) build --build-arg BINARY=hook -t $(REGISTRY)/$(DOCKER_REPO)/hook -f $(DEPLOY_DOCKERFILE) .
-	$(DOCKER) tag $(REGISTRY)/$(DOCKER_REPO)/hook $(REGISTRY)/$(DOCKER_REPO)/hook:$(TAG)
+	$(DOCKER) build --build-arg BINARY=hook -t $(REGISTRY)/$(DOCKER_REPO)-hook -f $(DEPLOY_DOCKERFILE) .
+	$(DOCKER) tag $(REGISTRY)/$(DOCKER_REPO)-hook $(REGISTRY)/$(DOCKER_REPO)-hook:$(TAG)


Why is it - instead of / now?

Maybe we can also change DOCKER_REPO -> IMG_REPO?

Because it changed from prod.registry.devshift.net/osio-prod/ike-prow-plugins/test-keeper to quay.io/openshiftio/ike-prow-plugins-test-keeper as per the new repositories.

Here's the discussion - https://chat.openshift.io/developers/pl/tqzbcjx5f3nmdg7co9fn5kay5a

Thanks for the link. In such a case I think we should improve the naming here, as $(REGISTRY)/$(DOCKER_REPO)-hook is not really correct.

In our case this should be:

quay.io/openshiftio/rhel-ike-prow-plugins-work-in-progress

so

REGISTRY = quay.io

IMG_REPO = openshiftio

ike-prow-plugins- is a prefix of the plugin image name in such case.

bartoszmajsak · 2018-07-02T08:54:08Z

-  if [ -n "${DEVSHIFT_USERNAME}" -a -n "${DEVSHIFT_PASSWORD}" ]; then
-    docker login -u ${DEVSHIFT_USERNAME} -p ${DEVSHIFT_PASSWORD} ${REGISTRY}
+  if [ -n "${QUAY_USERNAME}" -a -n "${QUAY_PASSWORD}" ]; then
+    docker login -u ${QUAY_USERNAME} -p ${QUAY_PASSWORD} ${REGISTRY}


I would suggest using IMG_REPO prefix instead of QUAY. This will then have a proper meaning for any repo we use.

I like the idea for IMG_REPO but it's difficult for QUAY_USERNAME and QUAY_PASSWORD. Note that this come directly from credentials in CICO, so if we want to rename them, we should do it in these scripts.

bartoszmajsak · 2018-07-02T09:04:52Z

          containers:
          - name: hook
-            image: ${REGISTRY}/${DOCKER_REPO}/hook:${IMAGE_TAG}
+            image: ${REGISTRY}/${DOCKER_REPO}-hook:${IMAGE_TAG}


I don't think this is correct now. What is the value here for the generated yaml file?

bartoszmajsak · 2018-07-02T09:07:04Z

-	$(DOCKER) build --build-arg BINARY=hook -t $(REGISTRY)/$(DOCKER_REPO)/hook -f $(DEPLOY_DOCKERFILE) .
-	$(DOCKER) tag $(REGISTRY)/$(DOCKER_REPO)/hook $(REGISTRY)/$(DOCKER_REPO)/hook:$(TAG)
+	$(DOCKER) build --build-arg BINARY=hook -t $(REGISTRY)/$(DOCKER_REPO)-hook -f $(DEPLOY_DOCKERFILE) .
+	$(DOCKER) tag $(REGISTRY)/$(DOCKER_REPO)-hook $(REGISTRY)/$(DOCKER_REPO)-hook:$(TAG)


Thanks for the link. In such a case I think we should improve the naming here, as $(REGISTRY)/$(DOCKER_REPO)-hook is not really correct.

In our case this should be:

quay.io/openshiftio/rhel-ike-prow-plugins-work-in-progress

so

REGISTRY = quay.io

IMG_REPO = openshiftio

ike-prow-plugins- is a prefix of the plugin image name in such case.

hemanik · 2018-07-02T10:55:54Z

@jmelis can you please verify this PR mainly from the centos-ci publishing point of view?

bartoszmajsak · 2018-07-02T13:02:21Z

-	$(DOCKER) build --build-arg BINARY=hook -t $(REGISTRY)/$(DOCKER_REPO)/hook -f $(DEPLOY_DOCKERFILE) .
-	$(DOCKER) tag $(REGISTRY)/$(DOCKER_REPO)/hook $(REGISTRY)/$(DOCKER_REPO)/hook:$(TAG)
+	$(DOCKER) build --build-arg BINARY=hook -t $(REGISTRY)/$(IMG_REPO)/$(OC_PROJECT_NAME)-hook -f $(DEPLOY_DOCKERFILE) .
+	$(DOCKER) tag $(REGISTRY)/$(IMG_REPO)-hook $(REGISTRY)/$(IMG_REPO)/$(OC_PROJECT_NAME)-hook:$(TAG)


Shouldn't $(REGISTRY)/$(IMG_REPO)-hook be $(REGISTRY)/$(IMG_REPO)/$(OC_PROJECT_NAME)-hook now?

I think so too.

bartoszmajsak · 2018-07-02T13:03:36Z

  if [ -e "jenkins-env" ]; then
    cat jenkins-env \
-      | grep -E "(DEVSHIFT_TAG_LEN|DEVSHIFT_USERNAME|DEVSHIFT_PASSWORD|JENKINS_URL|GIT_BRANCH|GIT_COMMIT|BUILD_NUMBER|ghprbSourceBranch|ghprbActualCommit|BUILD_URL|ghprbPullId)=" \
+      | grep -E "(IMG_REPO_TAG_LEN|IMG_REPO_USERNAME|IMG_REPO_PASSWORD|JENKINS_URL|GIT_BRANCH|GIT_COMMIT|BUILD_NUMBER|ghprbSourceBranch|ghprbActualCommit|BUILD_URL|ghprbPullId)=" \


I don't know from where DEVSHIFT_* is coming. Might be env variable of Centos-CI. Can you make sure we are doing the right thing here?

That is correct, DEVSHIFT_USERNAME and DEVSHIFT_PASSWORD are coming from CentOS-CI, however if migrating to quay, we should be using QUAY_USERNAME and QUAY_PASSWORD instead. Can you gobally replace DEVSHIFT_USERNAME and DEVSHIFT_PASSWORD with their quay counterparts? also, please keep DEVSHIFT_TAG_LEN as is, that's a special variable that contains the expected length of git commit hash and can't be renamed.

Also, I recommend you to no longer use such a mechanism to load the env, but rather use this:

function load_jenkins_vars() { if [ -e "jenkins-env.json" ]; then eval "$(./env-toolkit load -f jenkins-env.json \ DEVSHIFT_TAG_LEN \ QUAY_USERNAME \ QUAY_PASSWORD \ JENKINS_URL \ GIT_BRANCH \ GIT_COMMIT \ BUILD_NUMBER \ ghprbSourceBranch \ ghprbActualCommit \ BUILD_URL \ ghprbPullId)" fi }

It's a much safer approach as it's resilient to env vars with spaces, newlines and special chars.

jmelis

I have added a few comments but the most important one is that there are two kind of quay repos, the public ones, and the private ones.

We are going to be building both CentOS based and RHEL based containers, and it's important to ensure that RHEL based containers are not pushed to public repos.

If it's a CentOS container it should be pushed to: $(REGISTRY)/$(IMG_REPO)/$(OC_PROJECT_NAME)-$<, but if it's a rhel container, it should be pushed to $(REGISTRY)/$(IMG_REPO)/rhel-$(OC_PROJECT_NAME)-$< and I don't see this logic anywhere.

You need to check for the TARGET env variable and check if $TARGET = rhel or not.

jmelis · 2018-07-02T13:16:12Z

  if [ -e "jenkins-env" ]; then
    cat jenkins-env \
-      | grep -E "(DEVSHIFT_TAG_LEN|DEVSHIFT_USERNAME|DEVSHIFT_PASSWORD|JENKINS_URL|GIT_BRANCH|GIT_COMMIT|BUILD_NUMBER|ghprbSourceBranch|ghprbActualCommit|BUILD_URL|ghprbPullId)=" \
+      | grep -E "(IMG_REPO_TAG_LEN|IMG_REPO_USERNAME|IMG_REPO_PASSWORD|JENKINS_URL|GIT_BRANCH|GIT_COMMIT|BUILD_NUMBER|ghprbSourceBranch|ghprbActualCommit|BUILD_URL|ghprbPullId)=" \


That is correct, DEVSHIFT_USERNAME and DEVSHIFT_PASSWORD are coming from CentOS-CI, however if migrating to quay, we should be using QUAY_USERNAME and QUAY_PASSWORD instead. Can you gobally replace DEVSHIFT_USERNAME and DEVSHIFT_PASSWORD with their quay counterparts? also, please keep DEVSHIFT_TAG_LEN as is, that's a special variable that contains the expected length of git commit hash and can't be renamed.

Also, I recommend you to no longer use such a mechanism to load the env, but rather use this:

function load_jenkins_vars() { if [ -e "jenkins-env.json" ]; then eval "$(./env-toolkit load -f jenkins-env.json \ DEVSHIFT_TAG_LEN \ QUAY_USERNAME \ QUAY_PASSWORD \ JENKINS_URL \ GIT_BRANCH \ GIT_COMMIT \ BUILD_NUMBER \ ghprbSourceBranch \ ghprbActualCommit \ BUILD_URL \ ghprbPullId)" fi }

It's a much safer approach as it's resilient to env vars with spaces, newlines and special chars.

jmelis · 2018-07-02T13:16:48Z

-  if [ -n "${DEVSHIFT_USERNAME}" -a -n "${DEVSHIFT_PASSWORD}" ]; then
-    docker login -u ${DEVSHIFT_USERNAME} -p ${DEVSHIFT_PASSWORD} ${REGISTRY}
+  if [ -n "${IMG_REPO_USERNAME}" -a -n "${IMG_REPO_PASSWORD}" ]; then
+    docker login -u ${IMG_REPO_USERNAME} -p ${IMG_REPO_PASSWORD} ${REGISTRY}


please replace with QUAY_...

jmelis · 2018-07-02T13:39:15Z

-	$(DOCKER) build --build-arg BINARY=hook -t $(REGISTRY)/$(DOCKER_REPO)/hook -f $(DEPLOY_DOCKERFILE) .
-	$(DOCKER) tag $(REGISTRY)/$(DOCKER_REPO)/hook $(REGISTRY)/$(DOCKER_REPO)/hook:$(TAG)
+	$(DOCKER) build --build-arg BINARY=hook -t $(REGISTRY)/$(IMG_REPO)/$(OC_PROJECT_NAME)-hook -f $(DEPLOY_DOCKERFILE) .
+	$(DOCKER) tag $(REGISTRY)/$(IMG_REPO)-hook $(REGISTRY)/$(IMG_REPO)/$(OC_PROJECT_NAME)-hook:$(TAG)


I think so too.

hemanik · 2018-07-03T08:55:42Z

You need to check for the TARGET env variable and check if $TARGET = rhel or not.

Here, we check if the $TARGET=rhel and the value is overriden for RHEL based containers otherwise the value is fetched from the Makefile.

@jmelis @bartoszmajsak, correct me if this is incorrect and we need to explictly check for the CentOS target in the cico_setup.sh script.

hemanik · 2018-07-03T09:09:19Z

Thanks @jmelis @bartoszmajsak for your review. I have incorporated the suggested changes.

MatousJobanek · 2018-07-13T15:23:35Z

[test]

MatousJobanek · 2018-07-13T15:41:32Z

-	$(DOCKER) build --build-arg BINARY=$< -t $(REGISTRY)/$(DOCKER_REPO)/$< -f $(DEPLOY_DOCKERFILE) .
-	$(DOCKER) tag $(REGISTRY)/$(DOCKER_REPO)/$< $(REGISTRY)/$(DOCKER_REPO)/$<:$(TAG)
+	$(DOCKER) build --build-arg BINARY=$< -t $(REGISTRY)/$(IMG_REPO)/$(OC_PROJECT_NAME)-$< -f $(DEPLOY_DOCKERFILE) .
+	$(DOCKER) tag $(REGISTRY)/$(IMG_REPO)/$(OC_PROJECT_NAME)-$< $(REGISTRY)/$(IMG_REPO)/$(OC_PROJECT_NAME)-$<:$(TAG)


Am I missing something or does the combination of:
https://github.com/arquillian/ike-prow-plugins/pull/218/files#diff-66536a55e28a60c7e24d2d4a43a8b89cR64
export IMG_REPO="openshiftio/rhel-"
with
$(REGISTRY)/$(IMG_REPO)/$(OC_PROJECT_NAME)-$<
results in
quay.io/openshiftio/rhel-/ike-prow-plugins-test-keeper
which is not correct

@MatousJobanek, you are correct. This is incorrect here.

@MatousJobanek, you are correct. This is incorrect.

@hemanik Is there a fix coming for it though? I can't see updates and would love to close this off.

@bartoszmajsak, will update it.

…ow-plugins into migration-to-quay

chore: migrate from devshift registry to quay.io

7598cb2

alien-ike added the size/M label Jul 2, 2018

hemanik requested a review from bartoszmajsak July 2, 2018 08:51

bartoszmajsak reviewed Jul 2, 2018

View reviewed changes

bartoszmajsak requested changes Jul 2, 2018

View reviewed changes

hemanik added 3 commits July 2, 2018 14:38

chore: rename prefix value from QUAY to IMG_REPO.

2d0e4b0

fix: DOCKER_REPO value and image names.

ee6328c

chore: rename DOCKER_REPO to IMG_REPO.

3c4eefa

bartoszmajsak requested changes Jul 2, 2018

View reviewed changes

jmelis suggested changes Jul 2, 2018

View reviewed changes

alien-ike added size/L and removed size/M labels Jul 3, 2018

fix: env variables for centos ci.

d29bd18

hemanik force-pushed the migration-to-quay branch from 236a9a9 to d29bd18 Compare July 3, 2018 08:55

Merge branch 'master' into migration-to-quay

e2ba1fa

alien-ike added size/M and removed size/L labels Jul 13, 2018

MatousJobanek reviewed Jul 13, 2018

View reviewed changes

hemanik added 4 commits August 21, 2018 22:21

merges upstream master.

2dabe34

fix: update IMG_REPO value.

7d0fa3e

Merge branch 'migration-to-quay' of https://github.com/hemanik/ike-pr…

70fd76d

…ow-plugins into migration-to-quay

fix: IMG_REPO for local deployment.

50b2085

Conversation

hemanik commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

centos-ci commented Jul 2, 2018

Uh oh!

bartoszmajsak commented Jul 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartoszmajsak Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartoszmajsak Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemanik commented Jul 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmelis Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmelis left a comment

Choose a reason for hiding this comment

Uh oh!

jmelis Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemanik commented Jul 3, 2018

Uh oh!

hemanik commented Jul 3, 2018

Uh oh!

MatousJobanek commented Jul 13, 2018

Uh oh!

MatousJobanek Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hemanik commented Jul 2, 2018 •

edited

Loading

bartoszmajsak Jul 2, 2018 •

edited

Loading

bartoszmajsak Jul 2, 2018 •

edited

Loading

jmelis Jul 2, 2018 •

edited

Loading

jmelis Jul 2, 2018 •

edited

Loading

MatousJobanek Jul 13, 2018 •

edited

Loading