Skip to content

Update TestIstioConfig#3924

Merged
diemtvu merged 2 commits intoistio:masterfrom
diemtvu:fix_config_test
Mar 5, 2018
Merged

Update TestIstioConfig#3924
diemtvu merged 2 commits intoistio:masterfrom
diemtvu:fix_config_test

Conversation

@diemtvu
Copy link
Copy Markdown
Contributor

@diemtvu diemtvu commented Mar 3, 2018

The test was using hardcoded value for group and version. Change it to the one defined by ProtoSchema for the modernize IstioConfig.

@diemtvu diemtvu requested a review from a team March 3, 2018 00:42
@istio-merge-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: costinm

Assign the PR to them by writing /assign @costinm in a comment when ready.

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

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 3, 2018

Codecov Report

Merging #3924 into master will increase coverage by 1%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3924   +/-   ##
======================================
+ Coverage      76%     76%   +1%     
======================================
  Files         297     297           
  Lines       27210   27293   +83     
======================================
+ Hits        20537   20621   +84     
+ Misses       5348    5341    -7     
- Partials     1325    1331    +6
Impacted Files Coverage Δ
mixer/adapter/stackdriver/stackdriver.go 55% <0%> (-15%) ⬇️
.../adapter/solarwinds/internal/appoptics/batching.go 70% <0%> (-8%) ⬇️
mixer/adapter/prometheus/server.go 90% <0%> (-6%) ⬇️
...olarwinds/internal/papertrail/papertrail_logger.go 56% <0%> (-3%) ⬇️
mixer/adapter/rbac/rbac.go 10% <0%> (-1%) ⬇️
mixer/adapter/kubernetesenv/cache.go 83% <0%> (-1%) ⬇️
mixer/adapter/statsd/statsd.go 96% <0%> (-1%) ⬇️
mixer/adapter/kubernetesenv/kubernetesenv.go 68% <0%> (ø) ⬇️
mixer/adapter/servicecontrol/distValueBuilder.go 92% <0%> (+1%) ⬆️
mixer/adapter/fluentd/fluentd.go 78% <0%> (+2%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afd5911...66ab9e5. Read the comment docs.

@diemtvu
Copy link
Copy Markdown
Contributor Author

diemtvu commented Mar 3, 2018

/retest

@istio-testing
Copy link
Copy Markdown
Collaborator

@diemtvu: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-presubmit.sh 66ab9e5 link /test istio-presubmit
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.

Name: name,
Group: "config.istio.io",
Version: "v1alpha2",
Group: c.schema.Group + model.IstioAPIGroupDomain,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be ResourceGroup(c.schema).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It would cause circular dependency. I guess we will need to refactor the code a bit later.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok. ResourceGroup has to stay in crd. Let's keep this for now.

@diemtvu diemtvu merged commit da2219c into istio:master Mar 5, 2018
@diemtvu diemtvu deleted the fix_config_test branch March 15, 2018 20:09
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.

5 participants