Skip to content

comment out id in AoSIPPool ip_pool#3

Merged
that1guy15 merged 4 commits intomainfrom
ckim
Feb 23, 2022
Merged

comment out id in AoSIPPool ip_pool#3
that1guy15 merged 4 commits intomainfrom
ckim

Conversation

@kimcharli
Copy link
Copy Markdown
Contributor

When the id is assigned from display_name which may have space in the middle, it breaks url /api/resources/ip-pools/{id}.

Comment thread aos/resources.py Outdated
"tags": tags,
"display_name": name,
"id": name,
# "id": name, # generate to be compliant
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A number of workflows work off the basis that the ID is the same as the name for convenience. Instead of removing this all together, I suggest using replace() to replace spaces with "-".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Those scripts with name based should be modified anyway. If so, it would be straight forward to move to uuid instead another potential pitfalls in the future.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It does not make sense to full on remove the functionality. Now if you want to introduce the option to set ID with default being to set as the name then I am fine with that too.

One of the core goals of this project is to make automation simpler for end users and having a simple human readable name for as many elements as possible helps us do that. It helps reduce workflows and lookup API calls.

Comment thread aos/resources.py Outdated
"tags": tags,
"display_name": name,
"id": name,
# "id": name, # generate to be compliant
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It does not make sense to full on remove the functionality. Now if you want to introduce the option to set ID with default being to set as the name then I am fine with that too.

One of the core goals of this project is to make automation simpler for end users and having a simple human readable name for as many elements as possible helps us do that. It helps reduce workflows and lookup API calls.

@that1guy15 that1guy15 merged commit 3cc5ddf into main Feb 23, 2022
@kimcharli kimcharli deleted the ckim branch February 23, 2022 15:27
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