Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Operator oc support #1185
Operator oc support #1185
Changes from 17 commits
f7abe67
1ed62c4
b23e5cd
4914cbc
fd2087d
8b80a18
3d75af6
4f02879
6de05d8
a68b13c
fb27632
5bb81a5
d52187a
3fb4abf
46bc25e
643667c
3592c1c
7164b4a
fb49a05
1fd703e
4052ea7
31d742b
c8b0f73
0200cfe
bb0adf2
b7684f4
fd8e550
a6c3215
414defc
ea790bb
d0deef6
2ae4bab
c39936c
63ac759
556d0b9
6e28dc0
2c795be
c066f2b
cce5ea7
8caf5db
897065c
18f9797
9414a85
1e2ba7f
3f6089b
d7389bd
d246cd6
04a6bd7
8e8c62f
9660494
4865d1c
18a5cce
37345d3
cb74755
0353151
320ee3c
c86ad79
775e504
3c9e5c4
6f0ed87
a5d1143
87407ac
aae9fac
2c5a6d0
92648c4
c9b1b2d
443c831
1e10ece
f106737
06ccd09
1cbac51
8be0a21
e79e0b9
b9104d8
fb9f01d
afd927f
72a61bc
0258dd6
264bd03
a87e7df
8b3475f
5bc7816
648ef89
fb2cbf8
14527f4
77b7c85
a0fd62a
d5723da
c9c7a56
5fa62da
68e32aa
e03a7de
a1c3d97
5de4a42
3a7a17f
de8bc26
4fbdb9e
c051a1f
6442269
918b64d
643399c
31e779a
3f36e19
5c8d1fc
6301457
236255b
64a1010
aad99bc
a2653c4
0942ae7
6434e30
47ada8d
69a5c68
9455de2
40f281a
21a5e60
aa1b65d
d63af0d
d36b48c
741a837
8055e83
9612804
27bede2
0ee99d6
a6a5741
b560c12
9fd9d57
3806772
e1fe5b9
92358e7
63eb0f0
0d6eb82
af7ac81
df97d27
5b40cb2
36dee93
6c22de5
8fa7bf5
ec83696
703cf39
6014cc1
72c058a
79b374a
915a40f
89eaf4c
cd1157e
6a72f52
edfed88
af0a352
a96b49e
baacd59
40d4a3f
c45b81c
741e354
ba62d52
04e0542
1b153d6
bf31b52
9d773ef
d8b31cf
3132455
aba1b18
ad5f83a
a45e1e2
bd64172
10d273a
5732bb8
8e67f38
1f12b7f
fed7c70
9ef59b0
b14a228
433f293
7c5fbd6
5e07811
ecbdb4f
10dc57e
029dd7e
93188a5
7381d5f
e113083
9201676
b6e746f
34068f3
232c5a0
db7f39d
09a50a1
c71b621
45b715a
84a24ee
3c91631
76f6a78
24e5280
d7ba654
63a668f
70d5fae
fbb8de6
b31dd46
111be20
84eef67
7d27132
d2c11b8
bf45652
1180d93
3f778bc
b77cb30
f023a2b
e0b6187
43b0fd4
b187eac
1514bbd
fc3f788
ab2f262
3225c63
19ffd4c
fa9645a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although currently
BuildRouteForHeadService
will only readinstanceWithRouteEnabled
, it is better to perform a deep copy ofinstanceWithRouteEnabled
to avoid side effects in the future. I expect thatinstanceWithRouteEnabled
will be used in multiple tests. If the value will be different after each test, it will prevent the tests from covering the correct code paths.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh?
BuildRouteForHeadService
should never modify sourceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can refer to my comment https://github.com/ray-project/kuberay/pull/1185/files#r1255244213.
I would say that it is a good practice for every gopher to avoid side effects caused by future changes. For more details on this topic, you can refer to this article. In addition, you can find related comments frequently in Kubernetes source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I would agree that shallow copy is bad. In this particular case
BuildRouteForHeadService
is part of CR processing. If it modifies CR we have a much bigger problem. Also compare it with other tests, for example ingress testThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blublinsky, perhaps inlining the variable in the call to
BuildRouteForHead
might be a better solution. I think it should assuade @kevin85421 concerns and also slighly simplify the reading of the code (a subjective opinion)