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.

Sign up for GitHub

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

Jump to bottom

Misc-fixes #139

Merged
merged 4 commits into from
Dec 17, 2024
Merged

Misc-fixes #139

merged 4 commits into from
Dec 17, 2024

Conversation

jpinsonneau
Copy link
Contributor

Description

  • Add missing oc script to downstream dockerfile
  • Skip dependencies check to force versions

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
    • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
    • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
    • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
    • Standard QE validation, with pre-merge tests unless stated otherwise.
    • Regression tests only (e.g. refactoring with no user-facing change).
    • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

@jpinsonneau jpinsonneau mentioned this pull request Dec 16, 2024
Open
10 tasks
@codecov Codecov
Copy link

codecov bot commented Dec 16, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.66%. Comparing base (d541d19) to head (905b3fb).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #139   +/-   ##
=======================================
  Coverage   22.66%   22.66%           
=======================================
  Files          10       10           
  Lines        1337     1337           
=======================================
  Hits          303      303           
  Misses       1015     1015           
  Partials       19       19
Flag Coverage Δ
unittests 22.66% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

msherif1234
msherif1234 reviewed Dec 16, 2024
View reviewed changes
Makefile
GOFLAGS="" go install github.com/mikefarah/yq/v4@${YQ_VERSION}
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

@jpinsonneau why removed the above check just curious ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you already have yq, it will not update it.
That's an issue since we need at least 4.43.1

See https://redhat-internal.slack.com/archives/C03SHCD5C9M/p1734345188324779?thread_ts=1734110256.626139&cid=C03SHCD5C9M

@msherif1234
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 16, 2024
@openshift-ci openshift-ci bot removed the lgtm label Dec 16, 2024
msherif1234
msherif1234 reviewed Dec 16, 2024
View reviewed changes
Dockerfile.downstream Outdated
@@ -2,6 +2,9 @@
ARG TARGETARCH=amd64
ARG COMMIT

# Make kubectl & oc scripts available for copy
FROM brew.registry.redhat.io/openshift4/ose-cli-rhel9:v4.17.0-202412032103.p0.g13001b0.assembly.stream.el9 as ose-cli
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 have 4.18 images

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll take a look when I will have a working URL. For now I'm getting a not authorized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OlivierCazade seems like Konflux is passing on the image now but have a new error:

Failure snippet:
task build-container has the status "Failed":
File:     /opt/app-root/src/pkg/reconciler/taskrun/taskrun.go
  Line:     460

Could you PTAL ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

network-obser1e43682853f1a139e10e4d4475d7ce84-build-container-1[Duration: 24 minutes 59 seconds]
NETWORK-OBSER1E43682853F1A139E10E4D4475D7CE84-BUILD-CONTAINER-1

task build-container has failed: "step-build" exited with code 1
[prepare] 2024/12/17 10:16:52 Entrypoint initialization

[place-scripts] 2024/12/17 10:16:53 Decoded script /tekton/scripts/script-1-8zf45
[place-scripts] 2024/12/17 10:16:53 Decoded script /tekton/scripts/script-2-8999l
[place-scripts] 2024/12/17 10:16:53 Decoded script /tekton/scripts/script-3-rxmpk
[place-scripts] 2024/12/17 10:16:53 Decoded script /tekton/scripts/script-4-cjnhj
[place-scripts] 2024/12/17 10:16:53 Decoded script /tekton/scripts/script-5-vdf6q
[place-scripts] 2024/12/17 10:16:53 Decoded script /tekton/scripts/script-6-stqxn

[use-trusted-artifact] Using token for quay.io/redhat-user-workloads/ocp-network-observab-tenant/netobserv-operator/network-observability-cli-container
[use-trusted-artifact] Restored artifact quay.io/redhat-user-workloads/ocp-network-observab-tenant/netobserv-operator/network-observability-cli-container@sha256:923c68f58baf5c5de5ee5cef899df2c81dc59895307e4897b80819e453b5cd1b to /var/workdir/source
[use-trusted-artifact] Using token for quay.io/redhat-user-workloads/ocp-network-observab-tenant/netobserv-operator/network-observability-cli-container
[use-trusted-artifact] Restored artifact quay.io/redhat-user-workloads/ocp-network-observab-tenant/netobserv-operator/network-observability-cli-container@sha256:201013b1a3d4977ac0ac1cd8d2797ba9c8262b8cfcfab338ad3f42c1c35ac4dd to /var/workdir/cachi2
[use-trusted-artifact] 

[build] mkdir -p ~/.ssh
[build] if [ -e "/ssh/error" ]; then
[build]   #no server could be provisioned
[build]   cat /ssh/error
[build]   exit 1
[build] fi
[build] Error allocating host: timed out waiting for instance address
[build] 
[build] Context info:
[build]   Platform: linux/arm64
[build]   File:     /opt/app-root/src/pkg/reconciler/taskrun/taskrun.go
[build]   Line:     460
[build] 

container step-build has failed  : [{"key":"StartedAt","value":"2024-12-17T10:17:05.341Z","type":3}]

msherif1234
msherif1234 reviewed Dec 16, 2024
View reviewed changes
Dockerfile.downstream
@@ -2,6 +2,9 @@
ARG TARGETARCH=amd64
ARG COMMIT

# Make kubectl & oc scripts available for copy
FROM brew.registry.redhat.io/openshift4/ose-cli-rhel9:v4.17.0-202412032103.p0.g13001b0.assembly.stream.el9 as ose-cli

# Build the manager binary
FROM brew.registry.redhat.io/rh-osbs/openshift-golang-builder:v1.22.5-202407301806.g4c8b32d.el9 as builder
Copy link
Contributor

Choose a reason for hiding this comment

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

we also have 1.23 golang builder images

@jpinsonneau jpinsonneau force-pushed the misc-fixes branch 2 times, most recently from 9bb540c to 1ab1c48 Compare December 16, 2024 16:00
memodi
memodi reviewed Dec 16, 2024
View reviewed changes
Copy link

@memodi memodi left a comment

Choose a reason for hiding this comment

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

@jpinsonneau @msherif1234 - how is this supposed to work e2e, am I (as customer) supposed to update the yq version before using CLI 1.8? I don't think we'd expect customers to install dependencies using make targets.

@jpinsonneau
Copy link
Contributor Author

@jpinsonneau @msherif1234 - how is this supposed to work e2e, am I (as customer) supposed to update the yq version before using CLI 1.8? I don't think we'd expect customers to install dependencies using make targets.

That's just for us indeed. Check #140

@jpinsonneau
Copy link
Contributor Author

/retest

@msherif1234
Copy link
Contributor

/lgtm
we can defer bumping to 4.18 images for future PRs

@openshift-ci openshift-ci bot added the lgtm label Dec 17, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Dec 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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-merge-bot openshift-merge-bot bot merged commit 8c5a132 into netobserv:main Dec 17, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@memodi memodi memodi left review comments

@msherif1234 msherif1234 msherif1234 left review comments

@OlivierCazade OlivierCazade Awaiting requested review from OlivierCazade

Assignees

@msherif1234 msherif1234

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jpinsonneau @msherif1234 @memodi