-
Notifications
You must be signed in to change notification settings - Fork 10
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
Updated go.mod file with necessary dependencies to improve functionality #38
Updated go.mod file with necessary dependencies to improve functionality #38
Conversation
1d63a75
to
e57963c
Compare
@@ -117,7 +111,4 @@ func TestUpdateToken(t *testing.T) { | |||
if token1 == token2 || token1Expire == token2Expire { | |||
t.Error("Token failed to updated") | |||
} | |||
if *tp.config.Client.CredsToken != token2 { | |||
t.Error("Failed to update client token to token2") | |||
} |
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.
Why remove those test cases?
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.
Hi @zetaj ,
From the given nested structure *tp.config.Client.CredsToken
, it looks like the CredsToken
field has been removed from the recent upgraded packages of github.com/AthenZ/athenz/clients/go/zms
. As a result, it is no longer supported, hence our test cases are failing. Therefore, we removed the CredsToken
field.
@@ -81,9 +81,6 @@ func TestToken(t *testing.T) { | |||
if token1 != token2 || token1Expire != token2Expire { | |||
t.Error("Token updated when not expired") | |||
} | |||
if *tp.config.Client.CredsToken != token2 { |
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.
Do we know the reason we are removing this ?
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.
Hi @bhaskaramVaranasi ,
From the given nested structure *tp.config.Client.CredsToken
, it looks like the CredsToken
field has been removed from the recent upgraded packages of github.com/AthenZ/athenz/clients/go/zms
. As a result, it is no longer supported, hence our test cases are failing. Therefore, we removed the CredsToken
field.
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.
Hi @bhaskaramVaranasi @zetaj ,
Client.CredsToken
has been removed from github.com/AthenZ/[email protected]
version. The last it was used in
github.com/AthenZ/[email protected]
version.
Below is the screenshot of the Client.CredsToken being used for the final time under client.go file
pkg/cron/cron.go
Outdated
@@ -96,7 +96,7 @@ func notifyOnErr(err error, backoffDelay time.Duration) { | |||
// RequestCall - ZMS call for update crons | |||
func (c *Cron) requestCall() error { | |||
master := false | |||
domains, etag, err := c.zmsClient.GetSignedDomains("", "true", "", &master, c.etag) | |||
domains, etag, err := c.zmsClient.GetSignedDomains("", "true", "", &master,&master, c.etag) |
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.
Do we need to pass in the &master
twice ? what is it being used for ?
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.
Hi @bhaskaramVaranasi,
Please find the attached screenshot of the function GetSignedDomains()
.
The conditions
field has been added as a parameter to the updated package. So to accomodate this, we are sending it as a boolean set to false
.
pkg/controller/controller.go
Outdated
@@ -296,7 +296,7 @@ func (c *Controller) sync(domain string) error { | |||
func (c *Controller) zmsGetSignedDomains(domain string) (*zms.SignedDomains, bool, error) { | |||
d := zms.DomainName(domain) | |||
master := false | |||
signedDomain, _, err := c.zmsClient.GetSignedDomains(d, "", "", &master, "") | |||
signedDomain, _, err := c.zmsClient.GetSignedDomains(d, "", "", &master,&master, "") |
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.
Same as this comment: https://github.com/AthenZ/k8s-athenz-syncer/pull/38/files#r1618011313 .
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.
Hi @bhaskaramVaranasi ,
Please find the attached screenshot of the function GetSignedDomains()
.
The conditions
field has been added as a parameter to the updated package. So to accomodate this, we are sending it as a boolean set to false
.
pkg/cron/cron.go
Outdated
@@ -96,7 +96,7 @@ func notifyOnErr(err error, backoffDelay time.Duration) { | |||
// RequestCall - ZMS call for update crons | |||
func (c *Cron) requestCall() error { | |||
master := false | |||
domains, etag, err := c.zmsClient.GetSignedDomains("", "true", "", &master, c.etag) | |||
domains, etag, err := c.zmsClient.GetSignedDomains("", "true", "", &master,&master, c.etag) |
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.
Would you give more information regarding this change?
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.
Hi @zetaj ,
Please find the attached screenshot of the function GetSignedDomains()
.
The conditions
field has been added as a parameter to the updated package. So to accomodate this, we are sending it as a boolean set to false
.
pkg/controller/controller.go
Outdated
@@ -296,7 +296,7 @@ func (c *Controller) sync(domain string) error { | |||
func (c *Controller) zmsGetSignedDomains(domain string) (*zms.SignedDomains, bool, error) { | |||
d := zms.DomainName(domain) | |||
master := false | |||
signedDomain, _, err := c.zmsClient.GetSignedDomains(d, "", "", &master, "") | |||
signedDomain, _, err := c.zmsClient.GetSignedDomains(d, "", "", &master,&master, "") |
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.
Please give more context/information regarding this change.
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.
Hi @zetaj ,
Please find the attached screenshot of the function GetSignedDomains()
.
The conditions
field has been added as a parameter to the updated package. So to accomodate this, we are sending it as a boolean set to false
.
pkg/apis/athenz/v1/types.go
Outdated
|
||
Items []AthenzDomain `json:"items"` | ||
} | ||
/* |
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.
Looks like this whole file shows changes, can you please check why and fix it so that only the changes are made to the specific lines you changed ?
c073dab
to
95beb31
Compare
@@ -96,7 +96,8 @@ func notifyOnErr(err error, backoffDelay time.Duration) { | |||
// RequestCall - ZMS call for update crons | |||
func (c *Cron) requestCall() error { | |||
master := false | |||
domains, etag, err := c.zmsClient.GetSignedDomains("", "true", "", &master, c.etag) |
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.
What is the conditions
variable being used here for ?
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.
Hi @bhaskaramVaranasi , Please find the screenshot below.
This conditions
field is one of the input parameter required to form the url
. The value for this is kept as false
since the other variable master
was set to false
in the same way we are setting this conditions
field to false
.
Below screenshot is from cron.go
Below screenshot is from client.go, this file is from the package [email protected]/clients/go/zms/client.go
Signed-off-by: UdayMadhav88 <[email protected]> Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
…kage of Athenz Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
Signed-off-by: umothadaka <[email protected]>
d829d83
to
0e5ef1b
Compare
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.
Please have Bhaskaram doing 2nd review.
Goal
To update go.mod file with the latest versions of all dependencies to improve functionality.
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.