Skip to content
This repository has been archived by the owner on Aug 16, 2024. It is now read-only.

chore(release scripts): update release scripts due to changes in sidecar lifecycle #483

Merged
merged 7 commits into from
Oct 13, 2021

Conversation

vitaliy-guliy
Copy link
Contributor

Signed-off-by: Vitaliy Gulyy [email protected]

What does this PR do?

Updates the release scripts.
Gives up using base_images file with list of sidecar images. To check the sidecar Dockerfiles, directory dockerfiles is used.

Screenshot/screencast of this PR

What issues does this PR fix or reference?

eclipse-che/che#20603
a part of eclipse-che/che#19695

How to test this PR?

  • Checkout the repository
  • Run scripts locally

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@benoitf
Copy link
Contributor

benoitf commented Oct 8, 2021

Hello, I'm not sure to understand why digest stuff is removed ?
AFAIK short sha1 tags are not digests

make-release.sh Outdated Show resolved Hide resolved
make-release.sh Outdated Show resolved Hide resolved
@vitaliy-guliy
Copy link
Contributor Author

Hello, I'm not sure to understand why digest stuff is removed ? AFAIK short sha1 tags are not digests

I cannot say the stuff was removed. Instead of keeping all digests in one file we can get them from the sidecar dockerfiles.
Let's take a look at the dockerfile

ARG BASE_IMAGE="docker.io/maven:3.6.1-jdk-8"
FROM docker.io/maven@sha256:69b40237b342fee9bf996b81110f4dab250a4bc6a2ee52866965101eda066324

Line 12 contains digest of the necessary tag ( line 10 ).

Short sha1 we use only to tag che-* sidecar images.

@benoitf
Copy link
Contributor

benoitf commented Oct 11, 2021

@vitaliy-guliy but digest you're showing are the one of the parent, not the one of the current devfile images

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

AFAIK we should still replace tag by digest if --use-digests option is provoded

@vitaliy-guliy
Copy link
Contributor Author

vitaliy-guliy commented Oct 12, 2021

AFAIK we should still replace tag by digest if --use-digests option is provoded

Corresponding code reverted.

I built the registry with ./build.sh --use-digests, opened terminal inside built container and checked java-maven devfile.

bash-5.0# cat /var/www/html/devfiles/java-maven/devfile.yaml
---
apiVersion: 1.0.0
metadata:
  generateName: java-maven-
projects:
  -
    name: console-java-simple
    source:
      type: git
      location: "https://github.com/che-samples/console-java-simple"
      branch: java1.11
components:
  -
    type: chePlugin
    id: redhat/java/latest
    preferences:
      java.server.launchMode: Standard
  -
    type: dockerimage
    alias: maven
    image: "quay.io/eclipse/che-java11-maven@sha256:cc9e3cb0b00357ed118c130ce9f8e8e4724c718f8e2a1bd294f81696116d7b32" # tag: quay.io/eclipse/che-java11-maven:ce0526f
    env:
      - name: MAVEN_CONFIG
        value: ""
      - name: MAVEN_OPTS
        value: "-XX:MaxRAMPercentage=50 -XX:+UseParallelGC -XX:MinHeapFreeRatio=10
          -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
          -Dsun.zip.disableMemoryMapping=true -Xms20m -Djava.security.egd=file:/dev/./urandom
          -Duser.home=/home/user"
      - name: JAVA_OPTS
        value: "-XX:MaxRAMPercentage=50 -XX:+UseParallelGC -XX:MinHeapFreeRatio=10
          -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90
          -Dsun.zip.disableMemoryMapping=true -Xms20m -Djava.security.egd=file:/dev/./urandom"

...

ARG USE_DIGESTS=false
ENV USE_DIGESTS=${USE_DIGESTS}
Copy link
Contributor

Choose a reason for hiding this comment

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

hello, any reason to remove 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.

It looks like redundant, as it works fine only with ARG USE_DIGESTS=false ( like in the Dockerfile )
Do we really need it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

because we no longer build the devfile registry with hardcoded digests. Instead, the registry reads the digests at run time from the operator's csv env var. See https://issues.redhat.com/browse/CRW-1157

Copy link
Contributor

Choose a reason for hiding this comment

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

we should also remove ARG USE_DIGESTS=false since we don't use it anymore .... right?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://issues.redhat.com/browse/CRW-1157 should have been closed only if upstream was cleaned up as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, redhat-developer/devspaces-images#123 for the fix

Copy link
Contributor

Choose a reason for hiding this comment

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

https://issues.redhat.com/browse/CRW-1157 should have been closed only if upstream was cleaned up as well

Perhaps... or we should have had a matching GH issue for upstream :D Either way, you're right, the followup cleanup work wasn't done at that time.

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

LGTM

@nickboldt
Copy link
Contributor

AFAIK we should still replace tag by digest if --use-digests option is provoded

But that's the exact opposite of using the env vars from the operator as per https://issues.redhat.com/browse/CRW-1157. Why go backwards to make builds more difficult?

build.sh Outdated Show resolved Hide resolved
@vitaliy-guliy
Copy link
Contributor Author

vitaliy-guliy commented Oct 13, 2021

@nickboldt all your comments are related to my last commit, in which I reverted the code, that replaces tags on digests when --use-digests is provided to build.sh script (was removed before).

@benoitf could you give some comments here and confirm that we need this functionality?

@benoitf
Copy link
Contributor

benoitf commented Oct 13, 2021

I'm happy to remove any stuff to rhel.Dockerfile to make it more compliant to Dockerfile

@vitaliy-guliy
Copy link
Contributor Author

I'm happy to remove any stuff to rhel.Dockerfile to make it more compliant to Dockerfile

We can do it within following updates.
Here is a question about having --use-digests option.

@benoitf
Copy link
Contributor

benoitf commented Oct 13, 2021

let's remove this option as it's only in rhel.Dockerfile and downstream is not using it anymore.
But it should have been deleted when downstream deleted it

@nickboldt
Copy link
Contributor

For final cleanup in downstream: redhat-developer/devspaces-images#123

@vitaliy-guliy vitaliy-guliy merged commit 96d61c1 into main Oct 13, 2021
@vitaliy-guliy vitaliy-guliy deleted the fix-docker-push branch October 13, 2021 14:49
@che-bot che-bot added this to the 7.38 milestone Oct 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants