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

Add td sync wf #146

Merged
merged 7 commits into from
Feb 16, 2023
Merged

Add td sync wf #146

merged 7 commits into from
Feb 16, 2023

Conversation

davidbelusky
Copy link
Contributor

@davidbelusky davidbelusky commented Feb 9, 2023

add TD sync function as a WF

sync_all_installed_devices = int(task["inputData"]["sync_all_installed_devices"])
data = {}
# If there is MOCK_UNICONFIG_URL_BASE in composefile use mock uniconfig to get installed devices
if MOCK_UNICONFIG_URL_BASE != "none":
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make this more robust i.e.

if MOCK_UNICONFIG_URL_BASE and MOCK_UNICONFIG_URL_BASE != "none"

also, is the "none" magic keyword needed ? wouldn't empty / undefined parameter be a more generic solution ?

Copy link
Contributor

Choose a reason for hiding this comment

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

... considering all the input params are essentially optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after discussion with Jozef i set here default value

  • MOCK_UNICONFIG_URL_BASE=http://uniconfig_mock:1080
    and commented it
    it looks fine to me that if someone want use it he will just uncomment this line

image

import requests
import util
from frinx_conductor_workers.frinx_rest import (
additional_uniconfig_request_params, uniconfig_url_base)

TOPOLOGY_DISCOVERY_BASE_URL = "http://topology-discovery:5000/api"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should come from env vars, just like the UC URL BASE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, changed. 👍

"Content-Type": "application/json",
"X-Auth-User-Roles": "admin-1",
}
MOCK_UNICONFIG_URL_BASE = os.getenv("MOCK_UNICONFIG_URL_BASE")
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is just for demo right ? and should go away eventually

I would add a comment here: // TODO deleteme after demo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it can be useful for local development and also for some demo for future that we can show these WF on mock UC server and we dont need necessary some simulated/real devices

To use mock uniconfig change in fm-workflow composefile env to MOCK_UNICONFIG_URL_BASE=http://uniconfig_mock:1080
"""
devices = task["inputData"]["devices"]
labels = task["inputData"]["labels"]
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this is safe in case labels are not in the input

should be something like task["inputData"].get("labels", "") ... with default empty value

this applies to all input params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was worried about this one also during development,
But labels are here always if input in WF is empty it will be just empty string labels="" so it is safe if user using workflow

@marosmars marosmars merged commit e8ea1ba into master Feb 16, 2023
@marosmars marosmars deleted the add_td_sync_wf branch February 16, 2023 09:14
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