Feature/custom faux mac prefix #329
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grafnu
reviewed
Jul 13, 2021
|
I think you're confusing positional arguments with flag arguments that just
happen to be in a specific order (but isn't strictly positional). I don't
think opting for the env var is the right answer here, I'd either 1) just
pile on to the existing how-it's-broken and so it's not bifurcating the
problem, or 2) we take the little bit of time to clean up the current
mechanism so it's not so persnickety about argument ordering. Personally
I'd just go with option #1, but if you feel strongly about it and/or don't
want to deal with it (valid position), then I can take a crack at fixing it
up before finalizing this PR (should be a fairly easy thing I can crank
through today).
…On Tue, Jul 13, 2021 at 10:37 AM henry54809 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .github/workflows/main.yaml
<#329 (comment)>:
> @@ -73,8 +73,12 @@ jobs:
- name: run python tests
run: testing/python_test test_fot
- name: setup fot stack
+ env:
+ FAUX_MAC_PREFIX: 9a:99:57:1e
bin/setup_stack has too many positional arguments already so I opted for
an env variable.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#329 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD5NP4JNTBW4KHQICLDTXR2XPANCNFSM5AJRZOJA>
.
|
|
PTAL |
grafnu
reviewed
Jul 13, 2021
bin/run_controller
Outdated
| @@ -153,6 +153,10 @@ if [[ -z $vxlan ]]; then | |||
| docker exec $CONTAINER ip link set data0 up | |||
| fi | |||
|
|
|||
| if [ $CLEAN_MAC_PREFIX != "$DEFAULT_CLEAN_MAC_PREFIX" ]; then | |||
| docker exec $CONTAINER /root/bin/rename_site_config $CLEAN_MAC_PREFIX | |||
There was a problem hiding this comment.
I think this should go inside controller_go -- basically just pass the CLEAN_MAC_PREFIX in (as per the docker_envs use above), and then inside of the controller sort out what to do about it. Generally better to avoid the "docker exec" solution because it's a lot harder to debug/diagnose/trace -- the only reason it's done for the data0 interface is because it has to be done externally after the container is created (effective race condition).
| source bin/stack_functions | ||
| set_faux_mac_prefix | ||
|
|
||
| intf_mac=$FAUX_MAC_PREFIX:8f:00 |
There was a problem hiding this comment.
what happens if FAUX_MAC_PREFIX is not defined? Doesn't this need a default somewhere?
There was a problem hiding this comment.
set_faux_mac_prefix defines the default
…ic into controller startup script
grafnu
approved these changes
Jul 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.