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

Failing test case for V1VerticalPodAutoscalerCrudTest #3775

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Failing test case for V1VerticalPodAutoscalerCrudTest #3775

merged 2 commits into from
Feb 1, 2022

Conversation

matthyx
Copy link
Contributor

@matthyx matthyx commented Jan 21, 2022 •
edited

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@matthyx
Copy link
Contributor Author

matthyx commented Jan 21, 2022

@rohanKanojia I have identified this serialization issue today... will work on it in the following days.

@matthyx matthyx changed the title Failing test case for V1VerticalPodAutoscalerCrudTest WIP Failing test case for V1VerticalPodAutoscalerCrudTest Jan 21, 2022
@rohanKanojia
Copy link
Member

Could you please create an issue also so that I can assign it to you ?

@matthyx
Copy link
Contributor Author

matthyx commented Jan 21, 2022

Could you please create an issue also so that I can assign it to you ?

#3776

@rohanKanojia rohanKanojia linked an issue Jan 21, 2022 that may be closed by this pull request
VerticalPodAutoscaler cannot load yaml with "controlledResources" #3776
Closed
@matthyx matthyx changed the title WIP Failing test case for V1VerticalPodAutoscalerCrudTest Failing test case for V1VerticalPodAutoscalerCrudTest Jan 27, 2022
@matthyx
Copy link
Contributor Author

matthyx commented Jan 27, 2022

@rohanKanojia done. Should I add an entry to the changelog?

@rohanKanojia
Copy link
Member

@matthyx : thanks, yes please.

@matthyx
Copy link
Contributor Author

matthyx commented Jan 28, 2022

@rohanKanojia ready :)

rohanKanojia
rohanKanojia reviewed Jan 28, 2022
View reviewed changes
...rc/test/java/io/fabric8/verticalpodautoscaler/test/crud/V1VerticalPodAutoscalerCrudTest.java
" containerPolicies:",
" - containerName: container",
" controlledResources: [\"cpu\", \"memory\"]",
" controlledValues: RequestsOnly"
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add an assert statement below to check whether resourceName is deserialized into VPA correctly?

Copy link
Member

Choose a reason for hiding this comment

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

Umm, sorry. I meant assertion for vpa.getSpec().getResourcePolicy().getContainerPolicies().get(0).getControlledResources() (field for which this fix is)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, done!

@sonarcloud
Copy link

sonarcloud bot commented Jan 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@matthyx
Copy link
Contributor Author

matthyx commented Jan 31, 2022

@rohanKanojia ready to review :)

@manusa manusa added this to the 5.12.1 milestone Feb 1, 2022
manusa
manusa approved these changes Feb 1, 2022
View reviewed changes
Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@manusa manusa merged commit bcfd4fd into fabric8io:master Feb 1, 2022
@matthyx matthyx deleted the vpa-crud branch February 1, 2022 14:28
manusa pushed a commit that referenced this pull request Feb 4, 2022
@matthyx @manusa
Failing test case for V1VerticalPodAutoscalerCrudTest (#3775)
5ce2a73 
* Failing test case for V1VerticalPodAutoscalerCrudTest

* Fix existingJavaType for controlledResources in VPA extension

(cherry picked from commit bcfd4fd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manusa manusa manusa approved these changes

@sunix sunix sunix approved these changes

@rohanKanojia rohanKanojia rohanKanojia approved these changes

@oscerd oscerd Awaiting requested review from oscerd

Assignees

@matthyx matthyx

Labels
None yet
Projects
None yet
Milestone
5.12.1
Development

Successfully merging this pull request may close these issues.

VerticalPodAutoscaler cannot load yaml with "controlledResources"
4 participants
@matthyx @rohanKanojia @manusa @sunix