search

openshift / ocm-container

Containerized environment for accessing OpenShift v4 clusters, packing necessary tools/scripts
Apache License 2.0
10 stars 63 forks source link

Fix the builder for oc-nodepp #211

Closed rendhalver closed 1 year ago

rendhalver commented 1 year ago

The builder for container for oc-nodepp was using the wrong URL. This fixes that. Thanks @xiaoyu74

xiaoyu74 commented 1 year ago

Hi @rendhalver - Thanks for the fix, besides the wrong URL, I think we should also replace select(.name|test("Linux_amd64")) to select(.name|test("Linux_x86_64")) in the Dockerfile below: 

RUN [[ $(platform_convert "@@PLATFORM@@" --amd64 --arm64) != "amd64" ]] && exit 0 || /bin/bash -c "curl -sSLf -O $(curl -sSLf ${NODEPP_URL} -o - | jq -r '.assets[] | select(.name|test("Linux_amd64")) | .browser_download_url') "

since the content getting from curl -sSLf ${NODEPP_URL} doesn't contain anyamd64 in the name value but x86_64 instead: 

curl -sSLf https://api.github.com/repos/mrbarge/oc-nodepp/releases/tags/v0.1.1 | grep -i name
  "upload_url": "https://uploads.github.com/repos/mrbarge/oc-nodepp/releases/93662846/assets{?name,label}",
  "tag_name": "v0.1.1",
  "name": "v0.1.1",
      "name": "checksums.txt",
      "name": "oc-nodepp_Darwin_arm64.tar.gz",
      "name": "oc-nodepp_Darwin_x86_64.tar.gz",
      "name": "oc-nodepp_Linux_arm64.tar.gz",
      "name": "oc-nodepp_Linux_x86_64.tar.gz",

Otherwise, it can cause the container build failed since the curl command not being able to extract the correct URL from the GitHub API response:

=> ERROR [oc-nodepp-builder 4/8] RUN [[ $(platform_convert "@@PLATFORM@@" --amd64 --arm64) != "amd64" ]] && exit 0 || /bin/bash -c "curl -sSLf -O $(curl -sSLf https://api.github.com/repos/mrbarge/oc-nodepp/releases/tags/v0.1.1 -o - | jq -r '.asse  0.7s
------
 > [oc-nodepp-builder 4/8] RUN [[ $(platform_convert "@@PLATFORM@@" --amd64 --arm64) != "amd64" ]] && exit 0 || /bin/bash -c "curl -sSLf -O $(curl -sSLf https://api.github.com/repos/mrbarge/oc-nodepp/releases/tags/v0.1.1 -o - | jq -r '.assets[] | select(.name|test("Linux_amd64")) | .browser_download_url') ":
0.642 curl: no URL specified!
0.642 curl: try 'curl --help' for more information
------
Dockerfile:351
--------------------
 349 |     # Download the binary tarball
 350 |     ## x86-native
 351 | >>> RUN [[ $(platform_convert "@@PLATFORM@@" --amd64 --arm64) != "amd64" ]] && exit 0 || /bin/bash -c "curl -sSLf -O $(curl -sSLf ${NODEPP_URL} -o - | jq -r '.assets[] | select(.name|test("Linux_amd64")) | .browser_download_url') "
 352 |     # arm-native
 353 |     RUN [[ $(platform_convert "@@PLATFORM@@" --amd64 --arm64) != "arm64" ]] && exit 0 || /bin/bash -c "curl -sSLf -O $(curl -sSLf ${NODEPP_URL} -o - | jq -r '.assets[] | select(.name|test("Linux_arm64")) | .browser_download_url') "
--------------------
ERROR: failed to solve: process "/bin/sh -c [[ $(platform_convert \"@@PLATFORM@@\" --amd64 --arm64) != \"amd64\" ]] && exit 0 || /bin/bash -c \"curl -sSLf -O $(curl -sSLf ${NODEPP_URL} -o - | jq -r '.assets[] | select(.name|test(\"Linux_amd64\")) | .browser_download_url') \"" did not complete successfully: exit code: 2
T0MASD commented 1 year ago

/lgtm tested works ok

 oc nodepp
    NODE                            MACHINE                                   ROLE       AGE     STATUS  CPU            MEMORY          
    ip-10-124-163-0.ec2.internal    xxxxxxp02ue1-dnglz-master-1                 🏛  master  2y364d          1137m (14%)    18281Mi (64%)   
    ip-10-124-220-9.ec2.internal    xxxxxxp02ue1-dnglz-master-2                 🏛  master  2y364d          1216m (15%)    17691Mi (62%)   
    ip-10-124-154-215.ec2.internal  xxxxxxp02ue1-dnglz-master-0                 🏛  master  2y364d          1439m (18%)    23182Mi (82%)   
    ip-10-124-153-254.ec2.internal  xxxxxxp02ue1-dnglz-infra-us-east-1a-bhjx2   🧱 infra   220d            315m (8%)      4487Mi (15%)    
    ip-10-124-216-219.ec2.internal  xxxxxxp02ue1-dnglz-infra-us-east-1c-bhhdc   🧱 infra   220d            394m (10%)     10185Mi (35%)   
    ip-10-124-185-70.ec2.internal   xxxxxxp02ue1-dnglz-infra-us-east-1b-j64jd   🧱 infra   220d            835m (21%)     11402Mi (40%)   
    ip-10-124-181-156.ec2.internal  xxxxxxp02ue1-dnglz-worker-us-east-1b-l5qwm  🐄 worker  374d            2410m (61%)    11358Mi (90%)🔥 
    ip-10-124-181-104.ec2.internal  xxxxxxp02ue1-dnglz-worker-us-east-1b-zrmz7  🐄 worker  2y357d          277m (7%)      5292Mi (42%)    
    ip-10-124-131-174.ec2.internal  xxxxxxp02ue1-dnglz-worker-us-east-1a-2lw5z  🐄 worker  2y364d          98m (2%)       4721Mi (37%)    
    ip-10-124-161-153.ec2.internal  xxxxxxp02ue1-dnglz-worker-us-east-1b-c2cvx  🐄 worker  683d            144m (3%)      2878Mi (23%)    
    ip-10-124-212-173.ec2.internal  xxxxxxp02ue1-dnglz-worker-us-east-1c-bhrdn  🐄 worker  2y167d          89m (2%)       2481Mi (20%)    
    ip-10-124-159-163.ec2.internal  xxxxxxp02ue1-dnglz-worker-us-east-1a-5g6qp  🐄 worker  2y167d          3724m (95%)🔥  8687Mi (70%)    
    ip-10-124-220-56.ec2.internal   xxxxxxp02ue1-dnglz-worker-us-east-1c-5vjlq  🐄 worker  577d            121m (3%)      2749Mi (21%)    
    ip-10-124-154-111.ec2.internal  xxxxxxp02ue1-dnglz-worker-us-east-1a-4bhqm  🐄 worker  2y364d          2843m (72%)    10251Mi (81%)   
    ip-10-124-222-81.ec2.internal   xxxxxxp02ue1-dnglz-worker-us-east-1c-p2jnw  🐄 worker  2y364d          90m (2%)       4808Mi (38%)
T0MASD commented 1 year ago

/lgtm

openshift-ci[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rendhalver, T0MASD, xiaoyu74 Once this PR has been reviewed and has the lgtm label, please assign iamkirkbater for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/openshift/ocm-container/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
iamkirkbater commented 1 year ago

I saw this after #212, but it looks like 212 is more correct, changing all the flags, so I've approved that one. I'm going to close this, and we can do the version upgrade in another PR.

/close

openshift-ci[bot] commented 1 year ago

@iamkirkbater: Closed this PR.

In response to [this](https://github.com/openshift/ocm-container/pull/211#issuecomment-1680632473): >I saw this after #212, but it looks like 212 is more correct, changing all the flags, so I've approved that one. I'm going to close this, and we can do the version upgrade in another PR. > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
rendhalver commented 1 year ago

I saw this after #212, but it looks like 212 is more correct, changing all the flags, so I've approved that one. I'm going to close this, and we can do the version upgrade in another PR.

/close

Thanks Kirk. More correct is definitely better. I am still learning how this works so that helps me understand that more. :)

I will put in the version update separately (which is what should be done anyway)