Skip to content

replace m4.large default with m5.large #850

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
Merged
openshift-merge-robot merged 1 commit into from
Jan 13, 2022
Merged

replace m4.large default with m5.large #850

openshift-merge-robot merged 1 commit into from
Jan 13, 2022

Conversation

@gregsheremeta
Copy link
Contributor

@gregsheremeta gregsheremeta commented Jan 12, 2022

There is no reason to ever use m4 over m5. m4's are
more expensive because it's legacy hardware. m5 is cheaper
and better.

/assign @csrwng

@sjenning
Copy link
Contributor

sjenning commented Jan 12, 2022

/lgtm

@sjenning
Copy link
Contributor

sjenning commented Jan 12, 2022

openshift installer did this last year openshift/installer#3853

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2022
@enxebre enxebre mentioned this pull request Jan 12, 2022
Merged
@enxebre
Copy link
Member

enxebre commented Jan 12, 2022 •
edited
Loading

Thanks! nit: could also update the comment in the api definition and regenerate the CRDs/docs // InstanceType is an ec2 instance type for node instances (e.g. m4-large).

@gregsheremeta
Copy link
Contributor Author

gregsheremeta commented Jan 12, 2022

/hold

could also update the comment in the api definition and regenerate the CRDs/docs

yeah just noticed that (m4.large vs m4-large -- not used to hyphens!)

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 12, 2022
@gregsheremeta
Copy link
Contributor Author

gregsheremeta commented Jan 12, 2022

/unhold

please re-review

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2022
@netlify
Copy link

netlify bot commented Jan 12, 2022 •
edited
Loading

✔️ Deploy Preview for hypershift-docs ready!

🔨 Explore the source changes: 1f354fd

🔍 Inspect the deploy log: https://app.netlify.com/sites/hypershift-docs/deploys/61e00dbd4c9b9b00084f9c7e

😎 Browse the preview: https://deploy-preview-850--hypershift-docs.netlify.app/reference/api

@enxebre
Copy link
Member

enxebre commented Jan 12, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2022
@alvaroaleman
Copy link
Contributor

alvaroaleman commented Jan 13, 2022

/hold
merge conflicts :(

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 13, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, gregsheremeta

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2022
@gregsheremeta
replace m4.large default with m5.large
1f354fd 
There is no reason to ever use m4 over m5. m4's are
more expensive because it's legacy hardware. m5 is cheaper
and better.

Updated docs and comments that referred to m5-large too.
@openshift-ci openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 13, 2022
@gregsheremeta
Copy link
Contributor Author

gregsheremeta commented Jan 13, 2022

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 13, 2022
@csrwng
Copy link
Contributor

csrwng commented Jan 13, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2022
@openshift-bot
Copy link

openshift-bot commented Jan 13, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link

openshift-bot commented Jan 13, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@gregsheremeta
Copy link
Contributor Author

gregsheremeta commented Jan 13, 2022 

[2022-01-13T14:19:15Z] NoClusters: No clusters in pool are ready to be claimed 
INFO[2022-01-13T15:49:15Z] Releasing cluster claims for test e2e-aws    
INFO[2022-01-13T15:49:15Z] Ran for 1h35m22s                             
ERRO[2022-01-13T15:49:15Z] Some steps failed:                           
ERRO[2022-01-13T15:49:15Z] 
  * could not run steps: step e2e-aws failed: failed to wait for the created cluster claim to become ready: timed out waiting for the condition

i guess CI is down or something

@openshift-bot
Copy link

openshift-bot commented Jan 13, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link

openshift-bot commented Jan 13, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

openshift-bot commented Jan 13, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

openshift-bot commented Jan 13, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 13, 2022

@gregsheremeta: all tests passed!

Full PR test history. Your PR dashboard.

Details

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

@openshift-merge-robot openshift-merge-robot merged commit 0c257b9 into openshift:main Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ironcladlou ironcladlou Awaiting requested review from ironcladlou

@sjenning sjenning Awaiting requested review from sjenning

1 more reviewer

@alvaroaleman alvaroaleman alvaroaleman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sjenning sjenning

@enxebre enxebre

@csrwng csrwng

@alvaroaleman alvaroaleman

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@gregsheremeta @sjenning @enxebre @alvaroaleman @csrwng @openshift-bot @openshift-merge-robot