-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Adding Examples for Python SDK to create FCR and FCR based Connections #7
Conversation
Once a team is onboarded to this SDK, it is expected that they will be responsible for keeping the code for their service up-to-date. We didn't have documentation to tell them how to do that; this adds those docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes requested. Please review comments as well as this summary:
- The comment strings are very distracting. Please reduce them as much as possible. No steps, notes, example usages. Give a very short summary of what is about to happen and then let the user read the code for better understanding. We can't maintain this level of commenting. Good code speaks for itself and doesn't need to be overly explained.
- The imports need to be cleaned up. Don't import anything
as module
because it loses the context of its namespace and becomes incredibly confusing. - The custom_serializer in utils, I couldn't find where it was used because of all of the distractions. Is it necessary?
This method performs an update operation on a specified Fabric Cloud Router by replacing | ||
the current name with a new one. It leverages the Equinix Fabric API to execute this change. | ||
|
||
Notes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the notes section of this comment. It's too much detail.
Construct a `CloudRouterPostRequest` object using the `module.CloudRouterPostRequest` class. | ||
This object contains the details required to create the Fabric Cloud Router. | ||
|
||
Notes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the notes sections of all comments.
@@ -0,0 +1,93 @@ | |||
from examples.services.fabricv4.utils import utils | |||
from equinix.services import fabricv4 as module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we import just as fabricv4 and not as module? Better for code readability to have a specific name other than an abstract one.
Returns: | ||
str: The UUID of the created Fabric Cloud Router. | ||
|
||
Steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to add steps in comments. They can read the code for this.
Returns: | ||
None: This method prints the details of the FCR to the console. | ||
|
||
Steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All steps from comments can be removed.
Returns: | ||
None: This method prints the update response to the console. | ||
|
||
Steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not going to comment on all steps removals but please go through and remove all references to notes and steps in comment strings.
from icecream import ic | ||
from examples.services.fabricv4.utils import utils | ||
from examples.services.fabricv4.oauth2 import configure_client_credentials | ||
from examples.services import fabricv4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an import collision here. Why are both being imported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that from examples.services import fabricv4
should be removable.
str: The UUID of the newly created connection, which can be used for further operations like querying the connection | ||
status, updating, or deleting the connection. | ||
|
||
Example Usage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove example usage from comment strings.
import os | ||
|
||
from examples.services.fabricv4.utils import utils | ||
from equinix.services import fabricv4 as module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check all imports and don't rename to module.
import os | ||
|
||
from examples.services.fabricv4.utils import utils | ||
from equinix.services import fabricv4 as module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need the as in import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a separate PR, let's add black
(or similar) to the GH actions to reduce the need for code-style feedback in reviews.
Similar action in another Equinix SDK: https://github.com/packethost/packet-python/blob/master/.github/workflows/test.yml#L15-L26
(this won't change the need for accuracy, process, comment, and documentation style review)
Once a team is onboarded to this SDK, it is expected that they will be responsible for keeping the code for their service up-to-date. We didn't have documentation to tell them how to do that; this adds those docs.
The `.openapi-generator` directory is already mentioned in `.gitignore`, but it had not been removed from the repository. In addition to removing that directory, this updates the Makefiles so that the `.openapi-generator` directory is explicitly removed whenever code is regenerated. We have to explicitly remove this directory because the `semantic-release/git` plugin [intentionally does not respect `.gitignore`](semantic-release/git#345), so these files will be re-committed on every release if they are not explicitly removed during code generation.
The `.openapi-generator` directory is already mentioned in `.gitignore`, but it had not been removed from the repository. In addition to removing that directory, this updates the Makefiles so that the `.openapi-generator` directory is explicitly removed whenever code is regenerated. We have to explicitly remove this directory because the `semantic-release/git` plugin [intentionally does not respect `.gitignore`](semantic-release/git#345), so these files will be re-committed on every release if they are not explicitly removed during code generation.
CXF-96059 Adding Examples for Python SDK:
1. Create FCR
2. GET FCR
3. Update FCR
4. Delete FCR
FCR to Port, AWS, Azure Connections:
1. Create Connection
2. GET Connection
3. Update Connection
4. Delete Connection