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

Use MinRevTTL instead of constant #3108

Closed
wants to merge 2 commits into from

Conversation

juagargi
Copy link
Contributor

@juagargi juagargi commented Sep 5, 2019

In SCIONLab, if we change the value path_mgmt.MinRevTTL from 10 to e.g. 20, some UTs in beacon_srv won't pass. Using the constant MinRevTTL instead of hardcoded 10 fixes them.


This change is Reviewable

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @juagargi)

a discussion (no related file):
Please fix the typo in the PR title.



go/beacon_srv/internal/ifstate/revoker_test.go, line 154 at r1 (raw file):

path_mgmt.MinRevTTL / time.Second

I think using path_mgmt.MinRevTTL.Seconds() would be good, or is there any advantage to use division?

@juagargi juagargi changed the title Use MivRevTTL instead of constant Use MinRevTTL instead of constant Sep 6, 2019
@scrye scrye added the i/needs investigation Issues/proposals that need to be confirmed/explored label Sep 6, 2019
Copy link
Contributor Author

@juagargi juagargi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Please fix the typo in the PR title.

Done.



go/beacon_srv/internal/ifstate/revoker_test.go, line 154 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
path_mgmt.MinRevTTL / time.Second

I think using path_mgmt.MinRevTTL.Seconds() would be good, or is there any advantage to use division?

Done.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @juagargi and @oncilla)


go/beacon_srv/internal/ifstate/revoker_test.go, line 196 at r2 (raw file):

			LinkType:     proto.LinkType_peer,
			RawTimestamp: util.TimeToSecs(time.Now().Add(-6 * time.Second)),
			RawTTL:       10,

please change here as well.

@juagargi
Copy link
Contributor Author

juagargi commented Sep 6, 2019

Closing this in favor of #3111

@juagargi juagargi closed this Sep 6, 2019
@juagargi juagargi deleted the minRevTTL_UT branch September 6, 2019 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i/needs investigation Issues/proposals that need to be confirmed/explored
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants