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

Replace 'vCPU' with 'vcpus' #345

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Replace 'vCPU' with 'vcpus' #345

merged 1 commit into from
Oct 31, 2023

Conversation

stejskalleos
Copy link
Contributor

GCE shows vCPU in the description of the machine, not the vcpus.

@app-sre-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@avitova
Copy link
Member

avitova commented Oct 4, 2023

Nice, thanks!
Could you please edit the commit message based on our docs? Sorry, it seems we do not have this document for front end, but the same rules should apply.

amirfefer
amirfefer reviewed Oct 4, 2023
View reviewed changes
src/Utils/querySearch.js Outdated
@@ -5,6 +5,7 @@ const OPERATORS = ['<', '>', '='];
const parseQuery = (query) => {
query = query.replace('memory', 'memory_mib');
query = query.replace('storage', 'storage_gb');
query = query.replace('vCPU', 'vcpus');
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest applying it also to vcpu, maybe use a case-insensitive query?

@ezr-ondrej
Copy link
Member

/retest

@ezr-ondrej
Copy link
Member

Could you please edit the commit message based on our docs? Sorry, it seems we do not have this document for front end, but the same rules should apply.

@stejskalleos feel free to use fix(HMS-2301): Replace 'vCPU' with 'vcpus' it will satisfy the linter, even tho it's bit of a cheating to use closed ticket, for such a small change I'd be perfectly fine with it :)

@ezr-ondrej ezr-ondrej force-pushed the stejskalleos-patch-1 branch 2 times, most recently from ff067e8 to 884aee4 Compare October 25, 2023 13:02
@stejskalleos @ezr-ondrej
fix: Replace 'vCPU' with 'vcpus'
1792ea6 
GCE shows vCPU in the description of the machine, not the vcpus.

Refs: HMS-2301
@ezr-ondrej ezr-ondrej force-pushed the stejskalleos-patch-1 branch from 884aee4 to 1792ea6 Compare October 25, 2023 13:25
@ezr-ondrej
Copy link
Member

/retest

@ezr-ondrej
Copy link
Member

I've addressed @avitova and @amirfefer concerns, so we can get this in, could you please re-review? 🙏

@ezr-ondrej
Copy link
Member

/retest

weird timeout 🤔

avitova
avitova approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@avitova avitova 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 to me, @amirfefer merge at will. :)

@ezr-ondrej
Copy link
Member

I'll go ahead and merge, Amir might not get to it and we addressed his comments, so we should be good :)
I'm happy to address any additional comments, if Amir has some :)

@ezr-ondrej ezr-ondrej merged commit 1c11b5b into RHEnVision:main Oct 31, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amirfefer amirfefer amirfefer left review comments

@avitova avitova avitova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stejskalleos @app-sre-bot @avitova @ezr-ondrej @amirfefer