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

WIP: cleanup and reformat bash syntax in 04 script #1482

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

WIP: cleanup and reformat bash syntax in 04 script #1482

wants to merge 1 commit into from

Conversation

tuminoid
Copy link
Member

Cleanup and reformat bash syntax in 04 script. Also requires fixes to lib/common.sh, but to keep PR contained, I rather took out the utility functions and put them in new lib/utils.sh, keeping lib/common.sh changes minimal.

Also, fixed some leftover issues I missed first time around in all root level scripts for consistency. Those should be nothing by indentation, == removals, or source -> . fixes, or shellcheck ignore corrections.

@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2024
@metal3-io-bot
Copy link
Collaborator

[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 ask for approval from tuminoid. 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:

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 11, 2024
@tuminoid tuminoid force-pushed the tuomo/cleanup-04-script branch from 2d8d41b to 016badd Compare December 11, 2024 13:28
@tuminoid
Copy link
Member Author

/test metal3-e2e-clusterctl-upgrade-test-main metal3-centos-e2e-integration-test-release-1-8 metal3-dev-env-integration-test-ubuntu-main

Running clusterctl upgrade tests as that seems to reveal all kinds of issues in 04 script before my changes.

@tuminoid tuminoid force-pushed the tuomo/cleanup-04-script branch from 016badd to 52a54e9 Compare December 11, 2024 13:32
@tuminoid
Copy link
Member Author

/test metal3-e2e-clusterctl-upgrade-test-main metal3-centos-e2e-integration-test-release-1-8 metal3-dev-env-integration-test-ubuntu-main

@tuminoid tuminoid force-pushed the tuomo/cleanup-04-script branch from 52a54e9 to c6fe580 Compare December 11, 2024 13:46
@tuminoid
Copy link
Member Author

/test metal3-e2e-clusterctl-upgrade-test-main metal3-centos-e2e-integration-test-release-1-8 metal3-dev-env-integration-test-ubuntu-main

@tuminoid
Copy link
Member Author

/retest

@tuminoid tuminoid force-pushed the tuomo/cleanup-04-script branch from c6fe580 to 1b89f77 Compare December 11, 2024 14:44
@tuminoid
Copy link
Member Author

/test metal3-e2e-clusterctl-upgrade-test-main metal3-centos-e2e-integration-test-release-1-8 metal3-dev-env-integration-test-ubuntu-main

@tuminoid tuminoid force-pushed the tuomo/cleanup-04-script branch 2 times, most recently from 5006bab to 5cade36 Compare December 11, 2024 15:35
@tuminoid
Copy link
Member Author

/test metal3-e2e-clusterctl-upgrade-test-main metal3-centos-e2e-integration-test-release-1-8 metal3-dev-env-integration-test-ubuntu-main

1 similar comment
@tuminoid
Copy link
Member Author

/test metal3-e2e-clusterctl-upgrade-test-main metal3-centos-e2e-integration-test-release-1-8 metal3-dev-env-integration-test-ubuntu-main

@tuminoid
Copy link
Member Author

This is probably hitting some issues of own making and also same issue trying to be solved in #1480

@tuminoid tuminoid force-pushed the tuomo/cleanup-04-script branch 2 times, most recently from bf0a0e8 to 3984b9b Compare December 11, 2024 18:10
@tuminoid
Copy link
Member Author

/test metal3-e2e-clusterctl-upgrade-test-main metal3-centos-e2e-integration-test-release-1-8 metal3-dev-env-integration-test-ubuntu-main

@tuminoid
Copy link
Member Author

tuminoid commented Dec 12, 2024 •
edited
Loading

Clusterctl is now solved, thanks to Lennart's PR. This also passes locally if we wait longer for things to complete, so the iterate logic which was double or tripled running commands by default is now fixed, ie. broken. Some debugging needed...

@tuminoid tuminoid force-pushed the tuomo/cleanup-04-script branch from 3984b9b to 679ceb8 Compare December 12, 2024 12:24
@tuminoid
Copy link
Member Author

/test metal3-centos-e2e-integration-test-release-1-8 metal3-dev-env-integration-test-ubuntu-main

@tuminoid
cleanup and reformat bash syntax in 04 script
d2ec209 
Cleanup and reforamt bash syntax in 04 script. Also requires fixes
to lib/common.sh, but to keep PR contained, I rather took out the
utility functions and put them in new lib/utils.sh.

Also, fixed some leftover issues I missed first time around in 01, 02
and 03 scripts for consistency.

Signed-off-by: Tuomo Tanskanen <tuomo.tanskanen@est.tech>
@tuminoid tuminoid force-pushed the tuomo/cleanup-04-script branch from 679ceb8 to d2ec209 Compare December 16, 2024 10:18
@tuminoid
Copy link
Member Author

This will be kept WIP and in hold until Jan, do not want to risk CI breakage for holiday season.

@tuminoid
Copy link
Member Author

/test metal3-centos-e2e-integration-test-release-1-9
Just CI testing, ignore.

@metal3-io-bot
Copy link
Collaborator

@tuminoid: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
metal3-dev-env-integration-test-ubuntu-main 679ceb8 link true /test metal3-dev-env-integration-test-ubuntu-main
metal3-centos-e2e-integration-test-release-1-8 679ceb8 link true /test metal3-centos-e2e-integration-test-release-1-8
metal3-centos-e2e-integration-test-release-1-9 d2ec209 link true /test metal3-centos-e2e-integration-test-release-1-9

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@metal3-io-bot
Copy link
Collaborator

@tuminoid: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
metal3-dev-env-integration-test-ubuntu-main 679ceb8 link true /test metal3-dev-env-integration-test-ubuntu-main
metal3-centos-e2e-integration-test-release-1-8 679ceb8 link true /test metal3-centos-e2e-integration-test-release-1-8
metal3-centos-e2e-integration-test-release-1-9 d2ec209 link true /test metal3-centos-e2e-integration-test-release-1-9

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. I understand the commands that are listed here.

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

@elfosardo elfosardo Awaiting requested review from elfosardo

@Sunnatillo Sunnatillo Awaiting requested review from Sunnatillo

Assignees
No one assigned
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tuminoid @metal3-io-bot