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

Make 'pkg/logs' platform-agnostic #6251

Merged
merged 6 commits into from
Nov 1, 2022
Merged

Make 'pkg/logs' platform-agnostic #6251

merged 6 commits into from
Nov 1, 2022

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented Oct 24, 2022 •
edited
Loading

What type of PR is this:
/kind task
/kind code-refactoring

What does this PR do / why we need it:
This PR is the outcome of a pair-programming session with @feloy; it makes the pkg/logs package agnostic of the platform,
similar to what was done in #6217.
Similarly, the platform is injected by the dependency injection system, depending on the --run-on flag.

Which issue(s) this PR fixes:
Fixes #6250

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:
odo (and specifically odo logs) should work like before :-)

@rm3l rm3l added the area/odo-on-podman Issues or PRs related to running odo against Podman label Oct 24, 2022
@openshift-ci openshift-ci bot added the kind/task Issue is actionable task label Oct 24, 2022
@netlify
Copy link

netlify bot commented Oct 24, 2022 •
edited
Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name Link
🔨 Latest commit df748a3
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/636028ee49ac08000993dd26

@openshift-ci openshift-ci bot requested review from dharmit and feloy October 24, 2022 10:06
@rm3l rm3l requested a review from valaparthvi October 24, 2022 10:06
@odo-robot
Copy link

odo-robot bot commented Oct 24, 2022 •
edited
Loading

Unit Tests on commit 1225ce6 finished successfully.
View logs: TXT HTML

@rm3l rm3l force-pushed the make_logs_platform_agnostic branch from c002bcd to 66089d3 Compare October 24, 2022 10:14
@odo-robot
Copy link

odo-robot bot commented Oct 24, 2022 •
edited
Loading

Validate Tests on commit 1225ce6 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Oct 24, 2022 •
edited
Loading

Kubernetes Tests on commit 1225ce6 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Oct 24, 2022 •
edited
Loading

Windows Tests (OCP) on commit 1225ce6 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Oct 24, 2022 •
edited
Loading

OpenShift Tests on commit 1225ce6 finished successfully.
View logs: TXT HTML

@rm3l rm3l force-pushed the make_logs_platform_agnostic branch 2 times, most recently from c1194cc to d329cb7 Compare October 24, 2022 10:30
@rm3l
Copy link
Member Author

rm3l commented Oct 24, 2022

/test v4.11-integration-e2e

@feloy
Copy link
Contributor

feloy commented Oct 28, 2022

/approve

@openshift-ci
Copy link

openshift-ci bot commented Oct 28, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy

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. Required by Prow. label Oct 28, 2022
@rm3l rm3l removed the area/odo-on-podman Issues or PRs related to running odo against Podman label Oct 28, 2022
dharmit
dharmit reviewed Oct 31, 2022
View reviewed changes
pkg/kclient/interface.go Outdated Show resolved Hide resolved
pkg/platform/interface.go Outdated Show resolved Hide resolved
rm3l and others added 6 commits October 31, 2022 18:01
@rm3l
Panic if platform for Exec is not supported yet
84906e2 
This will make the error more visible,
instead of hiding it.
@rm3l
Use a consistent receiver name in 'pkg/kclient.Client#DeploymentWatcher'
8ce14b7
@rm3l @feloy
Make 'pkg/logs/LogsClient' rely on the generic 'platform.Client'
24aebba 
Co-authored-by: Philippe Martin <phmartin@redhat.com>
@rm3l @feloy
Make 'pkg/component#Log' function rely on the generic 'platform.Client'
291ff7d 
Co-authored-by: Philippe Martin <phmartin@redhat.com>
@rm3l @feloy
Make the 'execHandler' in 'pkg/component' rely on the generic 'platfo…
5557758 
…rm.Client'

Co-authored-by: Philippe Martin <phmartin@redhat.com>
@rm3l
Move 'matchOwnerReferenceWithResources' unit tests from 'pkg/logs' to…
df748a3 
… 'pkg/kclient'

Those tests are specific to the Kubernetes implementation.
@rm3l rm3l force-pushed the make_logs_platform_agnostic branch from d329cb7 to df748a3 Compare October 31, 2022 19:58
@sonarcloud
Copy link

sonarcloud bot commented Oct 31, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rm3l
Copy link
Member Author

rm3l commented Oct 31, 2022 •
edited
Loading

Had to rebase and force-push to benefit from #6260; otherwise tests would not pass.

@dharmit
Copy link
Member

dharmit commented Nov 1, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 1, 2022
@feloy
Copy link
Contributor

feloy commented Nov 1, 2022

/test ci/prow/v4.11-integration-e2e

@openshift-ci
Copy link

openshift-ci bot commented Nov 1, 2022

@feloy: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test unit-and-validate-test
  • /test v4.10-images
  • /test v4.11-images
  • /test v4.11-integration-e2e
  • /test v4.12-images
  • /test v4.9-images

Use /test all to run all jobs.

In response to this:

/test ci/prow/v4.11-integration-e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@feloy
Copy link
Contributor

feloy commented Nov 1, 2022

/test v4.11-integration-e2e

@openshift-merge-robot openshift-merge-robot merged commit 918a4e9 into redhat-developer:main Nov 1, 2022
@rm3l rm3l deleted the make_logs_platform_agnostic branch December 1, 2022 16:34
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dharmit dharmit dharmit left review comments

@feloy feloy Awaiting requested review from feloy

@valaparthvi valaparthvi Awaiting requested review from valaparthvi

Assignees

@dharmit dharmit

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. area/refactoring Issues or PRs related to code refactoring kind/task Issue is actionable task lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make pkg/logs platform-agnostic
4 participants
@rm3l @feloy @dharmit @openshift-merge-robot