Fab 6024 rebranding #181
Fab 6024 rebranding #181
Conversation
mchutani-CS
requested review from
jgielstra-cs,
laguirre-cs,
mcriscolo-cs and
pkandarpa-cs
and removed request for
laguirre-cs
| ------------------------ | ||
|
|
||
| The cortex-python-builders library is an add-on library for use with the base cortex-python library that helps with building actions and skills. | ||
| The sensa-python-builders library is an add-on library for use with the base sensa-python library that helps with building actions and skills. |
There was a problem hiding this comment.
This is more of a product question, but do we know what the plan is for these two (extension) packages? Are we going to deprecate or update (rename):
cortex-python-builderscortex-python-profiles<--- I would presume this is to be functionally replaced by the Profiles SDK
Is there a backlog issue to address these packages? If not, then we should spin that off
laguirre-cs
left a comment
laguirre-cs
left a comment
There was a problem hiding this comment.
Looks good to me! I think the main things to update are the Copyright docstrings, the migration notes, and (probably) the environment variables
| else: | ||
| flow.request.url = flow.request.pretty_url.replace( | ||
| "http://cortex-internal.cortex.svc.cluster.local", cortex_endpoint | ||
| "http://cortex-internal.sensa.svc.cluster.local", cortex_endpoint |
There was a problem hiding this comment.
Interesting, this URI change implies we are 1) renaming the k8s namespace for installation to sensa, but 2) the service name is still cortex-internal (not sensa-internal). Is this the intention?
There was a problem hiding this comment.
That's a good catch. The namespace change isn't intentional.
I also feel like as part of the "customer facing" branding, the change of namespace, service names, etc. should be done across the product, but that isn't the scope of this issue right now
migration.md
Outdated
| @@ -1,67 +1,67 @@ | |||
| # Migration steps from `cortex-client` to `cortex-python` | |||
| # Migration steps from `sensa-client` to `sensa-python` | |||
There was a problem hiding this comment.
We should probably remote these existing Migration steps and other Deprecation notes to avoid confusing ourselves with backwards compatibility requirements.
My understanding is sensa-python doesn't have to be backwards compatible with cortex-python, so the earlier migration notes from cortex-client -> cortex-python are probably less relevant. We probably just need a short note saying that users of cortex-python should instead install sensa-python and rename instances of cortex to sensa..
| config = input("Cortex Personal Access Config: ") | ||
| config = input("Sensa Personal Access Config: ") | ||
| project = input("Project: ") | ||
| os.environ["CORTEX_PERSONAL_ACCESS_CONFIG"] = config |
There was a problem hiding this comment.
Are any environment variables going to be changing as well to start with SENSA_ ? There are scattered uses of these variables in the package.
There was a problem hiding this comment.
Not right now but eventually! This can be discussed but I think the iteration 1 is for the library name, its usage and distribution.
| cli_cfg = get_sensa_profile() | ||
| url, token = cli_cfg["url"], cli_cfg["token"] | ||
| # type: ignore # ignore until mypy properyly supports attr ... | ||
| return cls(url, version, token, **kwargs) |
There was a problem hiding this comment.
I can't comment on the right line on GitHub, but a just below this we should probably rename from_cortex_message() -> from_sensa_message()..
| __author__ = "TecnoTree" | ||
| __author_email__ = "TBD?" | ||
| __license__ = "Apache 2.0" | ||
| __copyright__ = "TecnoTree LLC. All Rights Reserved." |
There was a problem hiding this comment.
We need to make sure this copyright field matches the copyright statement included throughout the code - see the top of setup.py, sensa/client.py, etc. (this is one reason why I'm not fan of having the License string duplicated in every module).
|
|
||
| Users of `cortex-python` should install `sensa-python` instead. |
There was a problem hiding this comment.
I would suggest repurposing this file to only be a about migrating cortex-python -> sensa-python, because I don't think keeping the migration from cortex-client is useful anymore (if cortex-python is dead, why mention its predecessor?).
There was a problem hiding this comment.
Also, we make sure to say that cortex-python is deprecated here (maybe also in the README?)
This PR assumes:
https://github.com/CognitiveScale/sensa-pythonsensa-pythonNote: This PR should remain in the draft state until this piece of code isn't moved to it's own repository