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

Remove Volume.size_in_mb from api; improve pkg/queue tests #718

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aryan9600
Copy link
Contributor

@aryan9600 aryan9600 commented May 25, 2023

kind/bug
What this PR does / why we need it:

Remove Volume.size_in_mb since its unused and its non trivial to actually use it for its intended purpose.
Furthermore, remove t.Fatal calls from non test goroutines. Currently running go vet ./... would return:

pkg/queue/queue_test.go:50:6: call to (*T).Fatal from a non-test goroutine
pkg/queue/queue_test.go:94:6: call to (*T).Fatal from a non-test goroutine

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #553

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.09 ⚠️

Comparison is base (f421319) 58.33% compared to head (d83a1f9) 58.25%.

❗ Current head d83a1f9 differs from pull request most recent head f163021. Consider uploading reports for the commit f163021 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #718      +/-   ##
==========================================
- Coverage   58.33%   58.25%   -0.09%     
==========================================
  Files          56       56              
  Lines        2705     2702       -3     
==========================================
- Hits         1578     1574       -4     
- Misses        982      983       +1     
  Partials      145      145              
Impacted Files Coverage Δ
infrastructure/grpc/convert.go 39.88% <ø> (-0.45%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Remove `Volume.size_in_mb` since its unused and its non trivial to
actually use it for its intended purpose.

Signed-off-by: Sanskar Jaiswal <[email protected]>
Before this commit, running `go vet ./...` would return:
```
pkg/queue/queue_test.go:50:6: call to (*T).Fatal from a non-test goroutine
pkg/queue/queue_test.go:94:6: call to (*T).Fatal from a non-test goroutine
```

Signed-off-by: Sanskar Jaiswal <[email protected]>
@@ -142,8 +142,6 @@ message Volume {
VolumeSource source = 4;
// PartitionID is the uuid of the boot partition.
optional string partition_id = 5;
// Size is the size to resize this volume to.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's an optional field, but shouldn't we be deprecating this first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

We'd need to add reserved 6 and probably reserved "size_in_mb" to the definition of Volume.

@github-actions
Copy link
Contributor

This PR is stale because it has been open 30 days with no activity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting root_volume.size_in_mb seems to have no effect
4 participants