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.

Sign up for GitHub

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

Jump to bottom

[JUJU-338] Drop miscellaneous NewProviderAddress constructors in favour of a functional approach #13566

Merged
merged 3 commits into from Jan 5, 2022
Merged

[JUJU-338] Drop miscellaneous NewProviderAddress constructors in favour of a functional approach #13566

merged 3 commits into from Jan 5, 2022

Conversation

jack-w-shaw
Copy link
Member

@jack-w-shaw jack-w-shaw commented Dec 10, 2021 •
edited

There are many duplicate constructors for ProviderAddress's for specific cases and inputs. This isn't great practice.

Unify all these cases under a single constructor, a method of MachineAddress. And replace every occurrence of these old constructors with the new pattern

Then do the same with the old NewProviderAddresses constructors

Checklist

  • [ ] Requires a pylibjuju change
  • Added integration tests for the PR
  • [ ] Added or updated doc.go related to packages changed
  • Comments answer the question of why design decisions were made

QA steps

make static-analysis
go test ./...

Documentation changes

Please replace with any notes about how it affects current user workflow, CLI, or API.

Bug reference

No bug reference

@jack-w-shaw jack-w-shaw changed the title [JUJU-338] Drop Miscilaneous NewProviderAddress constructors in favour of a functional approach [JUJU-338] Drop miscellaneous NewProviderAddress constructors in favour of a functional approach Dec 10, 2021
manadart
manadart reviewed Dec 10, 2021
View reviewed changes
Copy link
Member

@manadart manadart left a comment

Choose a reason for hiding this comment

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

Looking good.

core/network/address_options.go Outdated Show resolved Hide resolved
core/network/address_options.go Outdated Show resolved Hide resolved
core/network/address_test.go Outdated Show resolved Hide resolved
@hpidcock
Copy link
Member

@jack-w-shaw please fill out the PR description following the template, even for WIP PRs.

@jack-w-shaw jack-w-shaw added 2.9 do not merge Even if a PR has been approved, do not merge the PR! labels Dec 13, 2021
@jack-w-shaw jack-w-shaw force-pushed the JUJU-338_drop_NewProviderAddressInSpace branch from 4c8d603 to f388976 Compare December 13, 2021 10:54
@jack-w-shaw
Remove NewProviderAddressInSpace constructor
519f63f 
Instead use the cleaner NewMachineAddress().AsProviderAddress
@jack-w-shaw
Remove NewProviderAddress constructor
b990d0c 
Instead use the cleaner NewMachineAddress().AsProviderAddress
@jack-w-shaw jack-w-shaw force-pushed the JUJU-338_drop_NewProviderAddressInSpace branch from f388976 to c2e7dd7 Compare December 13, 2021 13:57
@jack-w-shaw jack-w-shaw removed the do not merge Even if a PR has been approved, do not merge the PR! label Dec 13, 2021
manadart
manadart approved these changes Jan 5, 2022
View reviewed changes
Copy link
Member

@manadart manadart left a comment

Choose a reason for hiding this comment

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

This prepares us to add further members and options to ProviderAddress that will subsequently be hoisted out of InterfaceInfo into the Addresses slice, and represented in corresponding wire types.

Let's land it and proceed.

core/network/address.go Outdated
@@ -287,6 +287,18 @@ func (a MachineAddress) ValueWithMask() (string, error) {
return ipNet.String(), nil
}

// AsProviderAddress is used to construct a ProviderAddress out
Copy link
Member

Choose a reason for hiding this comment

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

s/out out of/from

core/network/address.go Outdated
@@ -307,6 +319,28 @@ func NewMachineAddress(value string, options ...func(AddressMutator)) MachineAdd
return addr
}

// MachinesAddresses is a slice of MachineAddress
Copy link
Member

Choose a reason for hiding this comment

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

s/MachinesAddresses/MachineAddresses

core/network/address.go
// NewMachineAddresses is a convenience function to create addresses
// from a variable number of string arguments, applying any supplied
// options to each address
func NewMachineAddresses(values []string, options ...func(AddressMutator)) MachineAddresses {
Copy link
Member

Choose a reason for hiding this comment

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

Something irks me about this constructor, but I can see it's ergonomic in a lot of places...

Copy link
Member Author

Choose a reason for hiding this comment

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

Admittedly using a slice of strings was a compromise. I decided that it would be better to keep options as ...func() consistent with the other constructors, meaning values needed to be changed to a slice.

If you can think of a better way I'd be happy to hear it 🙂

@jack-w-shaw
Remove NewProviderAddresses constructors
9c5bc6a 
Instead use the cleaner NewMachineAddresses().AsProviderAddresses
@jack-w-shaw jack-w-shaw force-pushed the JUJU-338_drop_NewProviderAddressInSpace branch from c2e7dd7 to 9c5bc6a Compare January 5, 2022 14:33
@jack-w-shaw
Copy link
Member Author

$$merge$$

@jujubot jujubot merged commit 7911a57 into juju:2.9 Jan 5, 2022
@wallyworld wallyworld mentioned this pull request Jan 12, 2022
Merged
jujubot added a commit that referenced this pull request Jan 13, 2022
@jujubot
Merge pull request #13607 from wallyworld/merge-2.9-20220112
80dc2b5 
#13607

Merge 2.9. 
2.9 was updated to use juju-db 4.4 - here we change that to 5.0

#13587 [JUJU-381] Fix test race in RaftLeaseRemoteSuite.TestSetAddress
#13588 [JUJU-349] Remove stub sentence from add-machine helper
#13570 [JUJU-256] MVP verify app health after controller/model upgrade
#13566 [JUJU-338] Drop miscellaneous NewProviderAddress constructors in favour of a functional approach
#13589 Update to latest version of Pebble
#13591 [JUJU-388] Ensure 'hostname -f' returns juju-assigned hostname on equinix metal
#13592 Remove the format2 test charm
#13593 The interactive version command doesn't need a controller
#13590 [JUJU-403] Remove txn watcher wrench
#13594 [JUJU-335] Expand functionality of ProviderAddress
#13601 [JUJU-402] Unit machine test fixes for fire-walled env (s390x)
#13603 [JUJU-412] Fix racy tests for the CAAS firewaller worker
#13604 [JUJU-413] Test Raft queue immediate dispatch instead of 1 batch per operation
#13602 [JUJU-106] Add support for mgo scram-sha256 auth; default to mongo 4.4 on bootstrap
#13599 [JUJU-396] juju info/find/download run without a controller
#13606 [JUJU-418] Fix some intermittent unit test failures
#13600 [JUJU-380] Shutdown application worker properly

Conflicts were in snap version change, imports, and removed code.
```
# Conflicts:
# agent/agent_test.go
# api/charmhub/client_test.go
# api/charmhub/data.go
# caas/kubernetes/provider/bootstrap_test.go
# cmd/juju/charmhub/data.go
# cmd/juju/charmhub/download.go
# cmd/juju/charmhub/download_test.go
# cmd/juju/charmhub/find.go
# cmd/juju/charmhub/find_test.go
# cmd/juju/charmhub/info.go
# cmd/juju/charmhub/info_test.go
# cmd/juju/charmhub/mocks/api_mock.go
# mongo/mongodfinder_test.go
# provider/maas/interfaces.go
# provider/maas/interfaces_test.go
# service/snap/snap_test.go
```
## QA steps

See PRs

[JUJU-381]: https://warthogs.atlassian.net/browse/JUJU-381?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-349]: https://warthogs.atlassian.net/browse/JUJU-349?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-256]: https://warthogs.atlassian.net/browse/JUJU-256?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-338]: https://warthogs.atlassian.net/browse/JUJU-338?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-403]: https://warthogs.atlassian.net/browse/JUJU-403?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-335]: https://warthogs.atlassian.net/browse/JUJU-335?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-402]: https://warthogs.atlassian.net/browse/JUJU-402?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-412]: https://warthogs.atlassian.net/browse/JUJU-412?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-413]: https://warthogs.atlassian.net/browse/JUJU-413?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-106]: https://warthogs.atlassian.net/browse/JUJU-106?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-396]: https://warthogs.atlassian.net/browse/JUJU-396?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[JUJU-388]: https://warthogs.atlassian.net/browse/JUJU-388?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
@jack-w-shaw jack-w-shaw deleted the JUJU-338_drop_NewProviderAddressInSpace branch May 26, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manadart manadart manadart approved these changes

Assignees
No one assigned
Labels
2.9
Projects
None yet
Milestone
No milestone
4 participants
@jack-w-shaw @hpidcock @manadart @jujubot