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

Export constants from token package #267

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mardim91
Copy link

Signed-off-by: Dimitrios Markou [email protected]

Signed-off-by: Dimitrios Markou <[email protected]>
@edwarnicke
Copy link
Member

@mardim91 I presume this is for your webhook work, correct?

If so, are the constants the only dependencies you'd be pulling into the webhook repo? I ask, because minimizing dependencies is always on my mind, and I'm curious is there is a way to do so here :)

@mardim91
Copy link
Author

@mardim91 I presume this is for your webhook work, correct?

If so, are the constants the only dependencies you'd be pulling into the webhook repo? I ask, because minimizing dependencies is always on my mind, and I'm curious is there is a way to do so here :)

@edwarnicke yes you are right the constatns from sdk-sriov repo is my only dependency. I agree maybe we should try to minimize the dependencies. We can just create those constants in some folder inside the webhook repo but this seems like duplicate code for the same constants or maybe we can have like a policy to put all the constants of NSM universe in a single repo and all the projects can use that repo to import constants. One candidate repo for that could be nsm/api. What do you think ?

@edwarnicke
Copy link
Member

Yeah... I share your concern about defining constants in multiple places... its asking for a bug later on when things are renamed.

For many constants, we put them in the api repo if they are shared across different downstream repos. Would that make sense here? If so, where would it make sense to put it in the api repo? Does the webhook currently depend on the API repo?

Like so many other things in code... this is going to be a game of trade-offs... so lets talk about the options and see what looks like the best among them :)

@mardim91
Copy link
Author

mardim91 commented Sep 27, 2021

Right now webhook only depends on the sdk repo. In case that we decide that api repo is the right place from strategic perspective to gather all the NSM constants then we can make it a dependency to webhook repo. Regarding where to put the constants that I do not know yet but definetely makes sense to me to put the constants for sriov token to the api repo.

@mardim91 mardim91 closed this Sep 27, 2021
@mardim91 mardim91 reopened this Sep 27, 2021
@edwarnicke
Copy link
Member

Right now webhook only depends on the sdk repo. In case that we decide that api repo is the right place from strategic perspective to gather all the NSM constants then we can make it a dependency to webhook repo. Regarding where to put the constants that I do not know yet but definetely makes sense to me to put the constants for sriov token to the api repo.

If the webhook depends on the sdk repo, it depends transitively on the api repo too :)

@Bolodya1997 - thoughts on the proper place to put this constant in the api repo?

@edwarnicke
Copy link
Member

edwarnicke commented Sep 28, 2021

@mardim91 Would it make sense to put SriovTokenLabel in `/pkg/api/networkservice/labels/sriov/constants.go' in https://github.com/networkservicemesh/api/

@mardim91
Copy link
Author

@mardim91 Would it make sense to put SriovTokenLabel in `/pkg/api/networkservice/labels/sriov/constants.go' in https://github.com/networkservicemesh/api/

@edwarnicke yes ok I will do that. I guess we should abandon this PR right ?

@edwarnicke
Copy link
Member

@edwarnicke yes ok I will do that. I guess we should abandon this PR right ?

@mardim91 Feel free to Close it and submit the PR to API :)

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.

2 participants