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.

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

Fix memory and disk-space option for create/update actions #7

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

nadjaheitmann
Copy link
Collaborator

No description provided.

@bastian-src
Copy link

Why are these two "options" and cpu_cores not?

@bastian-src
Copy link

bastian-src commented Jan 17, 2025

@nadjaheitmann
Copy link
Collaborator Author

Why are these two "options" and cpu_cores not?

Because I use the default option for cpu_cores but overwrite it for the other two. Otherwise it would generate naming that is generated from the apidoc param.

@nadjaheitmann
Copy link
Collaborator Author

Is it maybe related to this block?

https://github.com/ATIX-AG/hammer-cli-foreman-resource-quota/pull/7/files#diff-afa38be9bcc1e5e95246f119b14983cf3969b7e52c2dfea4b6e912428af0d6aaL54

You need this block to remove the default options that are generated.

@nadjaheitmann nadjaheitmann force-pushed the fix_quota_create_update_options branch from 7934db4 to 90d7ae2 Compare January 17, 2025 10:04
option '--disk-space', "Disk space", _('Maximum disk space in GiB'), attribute_name: :option_disk_gb, format: HammerCLI::Options::Normalizers::Number.new
option '--remove-memory-limit', :flag, _('Remove quota limit for memory')
option '--remove-disk-space-limit', :flag, _('Remove quota limit for disk space')
option '--remove-cpu-core-limit', :flag, _('Remove quota limit for CPU cores')

Choose a reason for hiding this comment

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

Should we use the plural here to be consistent with other mentions? remove-cpu-cores-limit

Copy link

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

Thanks Nadja, wording LGTM.

@nadjaheitmann nadjaheitmann force-pushed the fix_quota_create_update_options branch from 90d7ae2 to 389d837 Compare January 17, 2025 11:02
@nadjaheitmann nadjaheitmann force-pushed the fix_quota_create_update_options branch from 389d837 to f25f964 Compare January 17, 2025 11:26
@bastian-src
Copy link

I've tested creating a quota and passing disk-space and memory. Moreover, tested the exclusion of memory and remove-memory-limit etc.

LGTM, thanks @nadjaheitmann @maximiliankolb! 🐧


PS: The output looks really great, I like that:

hammer resource-quota show --id <ID>
Id:                      <ID>
Name:                    bastians-test-quota
Created at:              2025-01-20 07:49:51 UTC
Description:             
CPU cores:               
Memory [MiB]:            4096
Disk space [GiB]:        5
Assigned hosts:          0
Number of users:         0
Number of usergroups:    0
Number of missing hosts: 0
Utilization:             
    CPU cores:        
    Memory [MiB]:     0
    Disk space [GiB]: 0

@nadjaheitmann nadjaheitmann merged commit 809383c into main Jan 20, 2025
9 checks passed
@nadjaheitmann nadjaheitmann deleted the fix_quota_create_update_options branch January 20, 2025 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants