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

IPAddress default namespace #4883

Merged
merged 7 commits into from
Dec 8, 2023

Conversation

snaselj
Copy link
Contributor

@snaselj snaselj commented Nov 30, 2023

Closes: NaN

What's Changed

  • Assign default namespace to IPAddress, when no namespace is provided.

@snaselj snaselj marked this pull request as ready for review November 30, 2023 12:49
@snaselj
Copy link
Contributor Author

snaselj commented Nov 30, 2023

Without this fix, I was not able to create IPAddress instance without the namespace defined.

cc.: @whitej6

@glennmatthews
Copy link
Contributor

Are you trying to create an IPAddress without specifying either a namespace or a parent as context? I'm not entirely convinced that's a desirable workflow to permit. Can you provide some more explanation as to why this is needed?

@snaselj snaselj marked this pull request as draft November 30, 2023 13:39
@snaselj
Copy link
Contributor Author

snaselj commented Nov 30, 2023

Seems like expected behavior is to raise an exception Either a parent or a namespace must be provided..

However, this exception is not raised when using IPAddress.save().

Probably not necessary to do anything with that, closing this PR.

@snaselj snaselj closed this Nov 30, 2023
@snaselj snaselj deleted the u/snasej-fix-default-ipaddress-namespace branch November 30, 2023 13:53
@glennmatthews glennmatthews restored the u/snasej-fix-default-ipaddress-namespace branch December 6, 2023 14:15
@glennmatthews
Copy link
Contributor

Reopening this as it's been pointed out that Prefix creation automatically does a get_default_namespace() call if none is provided, so it's odd/inconsistent that IPAddress creation doesn't do the same.

@glennmatthews glennmatthews reopened this Dec 6, 2023
@gsnider2195
Copy link
Contributor

Reopening this as it's been pointed out that Prefix creation automatically does a get_default_namespace() call if none is provided, so it's odd/inconsistent that IPAddress creation doesn't do the same.

An IPAddress can't be created without a valid parent. This would be similar to setting a default form value on a required ForeignKey field and I don't think we do that anywhere else?

@glennmatthews
Copy link
Contributor

Reopening this as it's been pointed out that Prefix creation automatically does a get_default_namespace() call if none is provided, so it's odd/inconsistent that IPAddress creation doesn't do the same.

An IPAddress can't be created without a valid parent. This would be similar to setting a default form value on a required ForeignKey field and I don't think we do that anywhere else?

We do this already on Prefix and on VRF:

    namespace = models.ForeignKey(
        to="ipam.Namespace",
        on_delete=models.PROTECT,
        related_name="prefixes",
        default=get_default_namespace_pk,    <----------<<
    )
    namespace = models.ForeignKey(
        "ipam.Namespace",
        on_delete=models.PROTECT,
        related_name="vrfs",
        default=get_default_namespace_pk,   <---------<<
    )

It's also a usability issue for Jobs and Apps that are written for a single-namespace environment, having to either explicitly find the parent Prefix before creating the IP or else having to explicitly call get_default_namespace() on every IP creation.

@gsnider2195
Copy link
Contributor

Reopening this as it's been pointed out that Prefix creation automatically does a get_default_namespace() call if none is provided, so it's odd/inconsistent that IPAddress creation doesn't do the same.

An IPAddress can't be created without a valid parent. This would be similar to setting a default form value on a required ForeignKey field and I don't think we do that anywhere else?

We do this already on Prefix and on VRF:

    namespace = models.ForeignKey(
        to="ipam.Namespace",
        on_delete=models.PROTECT,
        related_name="prefixes",
        default=get_default_namespace_pk,    <----------<<
    )
    namespace = models.ForeignKey(
        "ipam.Namespace",
        on_delete=models.PROTECT,
        related_name="vrfs",
        default=get_default_namespace_pk,   <---------<<
    )

It's also a usability issue for Jobs and Apps that are written for a single-namespace environment, having to either explicitly find the parent Prefix before creating the IP or else having to explicitly call get_default_namespace() on every IP creation.

Makes sense. We'll need to do a get_closest_parent for the namespace that's returned by get_default_namespace and if a suitable parent isn't found it will still fail.

@gsnider2195 gsnider2195 added the emergent Unplanned work that is brought into a sprint after it's started. label Dec 7, 2023
@glennmatthews glennmatthews self-assigned this Dec 7, 2023
@glennmatthews glennmatthews marked this pull request as ready for review December 7, 2023 20:22
@glennmatthews glennmatthews merged commit b8a7c46 into develop Dec 8, 2023
22 checks passed
@glennmatthews glennmatthews deleted the u/snasej-fix-default-ipaddress-namespace branch December 8, 2023 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emergent Unplanned work that is brought into a sprint after it's started.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants