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

feat: Adding Examples for Python SDK to create FCR and FCR based Connections #7

Closed
wants to merge 35 commits into from

Conversation

jkallem-equinix
Copy link
Contributor

CXF-96059 Adding Examples for Python SDK:

  • Fabric Cloud Router
    1. Create FCR
    2. GET FCR
    3. Update FCR
    4. Delete FCR

FCR to Port, AWS, Azure Connections:

  • Connections
    1. Create Connection
    2. GET Connection
    3. Update Connection
    4. Delete Connection

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.
Copy link
Contributor

@thogarty thogarty left a 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:

  1. 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.
  2. 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.
  3. 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:
Copy link
Contributor

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:
Copy link
Contributor

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
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor

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:
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Member

@displague displague Aug 16, 2024

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)

ctreatma and others added 15 commits August 21, 2024 09:46
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.
@jkallem-equinix jkallem-equinix requested a review from a team as a code owner August 21, 2024 16:46
jkallem-equinix and others added 11 commits August 21, 2024 12:34
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.
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.

4 participants