Skip to content

🌱 Improve logging in e2e tests #2974

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
Open
adilGhaffarDev wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🌱 Improve logging in e2e tests #2974

adilGhaffarDev wants to merge 1 commit into from
+137 −148

Conversation

@adilGhaffarDev
Copy link
Member

@adilGhaffarDev adilGhaffarDev commented Nov 27, 2025 •
edited by tuminoid
Loading

What this PR does / why we need it:
Improve logging in e2e tests.

Fixes #

Checklist:

  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • E2E tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@metal3-io-bot
Copy link
Contributor

metal3-io-bot commented Nov 27, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details 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

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 27, 2025
@adilGhaffarDev
Copy link
Member Author

adilGhaffarDev commented Nov 27, 2025

/test metal3-centos-e2e-integration-test-main

lentzi90
lentzi90 reviewed Nov 27, 2025
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Looks good, just one suggestion about the naming

@adilGhaffarDev
Copy link
Member Author

adilGhaffarDev commented Nov 27, 2025

/hold
this needs changes in project infra
/test metal3-centos-e2e-integration-test-main

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2025
@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2025
@adilGhaffarDev
Copy link
Member Author

adilGhaffarDev commented Dec 9, 2025

/test metal3-centos-e2e-integration-test-main

@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2025
@adilGhaffarDev
Copy link
Member Author

adilGhaffarDev commented Dec 9, 2025

/test metal3-centos-e2e-integration-test-main

@adilGhaffarDev
Copy link
Member Author

adilGhaffarDev commented Dec 9, 2025

/test metal3-centos-e2e-integration-test-main

@adilGhaffarDev
Copy link
Member Author

adilGhaffarDev commented Dec 9, 2025

/test metal3-centos-e2e-integration-test-main

@tuminoid tuminoid requested a review from Copilot December 9, 2025 16:11
Copilot AI reviewed Dec 9, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves the logging and manifest collection in e2e tests by standardizing log collection paths and removing a redundant shell script in favor of Go-based manifest collection.

Key Changes:

  • Introduced structured log directory constants (workloadClusterLogCollectionBasePath and bootstrapClusterLogCollectionBasePath) to organize logs from different cluster types
  • Refactored log/manifest collection to include cluster names and operation phases (beforePivot, afterPivot, afterRePivot, beforeDelete) in directory paths for better organization
  • Removed scripts/fetch_manifests.sh as manifest collection is now handled entirely in Go code

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
test/e2e/pivoting.go Added new log path constants; updated log/manifest collection calls to use structured paths with cluster names; removed unused runtime import; replaced environment variable check with cluster proxy method; added bootstrap cluster log collection at all pivot stages
test/e2e/upgrade_clusterctl_test.go Updated log paths to use new constants and include cluster names in directory structure
test/e2e/logcollector.go Refactored FetchManifests to accept complete paths from callers; added "ironic" to manifest collection list; simplified FetchClusterLogs by moving cluster name appending to callers
test/e2e/common.go Added manifest and log collection for both bootstrap and target clusters before cleanup
scripts/fetch_manifests.sh Deleted obsolete shell script replaced by Go-based manifest collection

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@adilGhaffarDev
Improve logging in e2e tests
9ccaa36 
Signed-off-by: Muhammad Adil Ghaffar <[email protected]>
@adilGhaffarDev
Copy link
Member Author

adilGhaffarDev commented Dec 9, 2025

/test metal3-centos-e2e-integration-test-main

@tuminoid
Copy link
Member

tuminoid commented Dec 10, 2025

Please comment on and then resolve the comments, so it is easier to track what has been addressed.

@adilGhaffarDev
Copy link
Member Author

adilGhaffarDev commented Dec 10, 2025

Please comment on and then resolve the comments, so it is easier to track what has been addressed.

all comments were valid, happened because of copy pasting code, all of them are fixed.
I moved to logging to a separation function so there is no copy pasting now. I will ask copilot for re review.

Copilot AI reviewed Dec 10, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tuminoid
Copy link
Member

tuminoid commented Dec 12, 2025

/retest
/test build

@tuminoid
Copy link
Member

tuminoid commented Dec 12, 2025

/override build

@metal3-io-bot
Copy link
Contributor

metal3-io-bot commented Dec 12, 2025

@tuminoid: Cannot update PR status for context build

Details

In response to this:

/override build

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@honza honza Awaiting requested review from honza

+1 more reviewer

@lentzi90 lentzi90 lentzi90 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@adilGhaffarDev @metal3-io-bot @tuminoid @lentzi90