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

Allow multiple constraints flag #17144

Merged
merged 6 commits into from
Jun 5, 2024
Merged

Allow multiple constraints flag #17144

merged 6 commits into from
Jun 5, 2024

Conversation

amandahla
Copy link
Contributor

@amandahla amandahla commented Apr 3, 2024 •
edited

Checklist

  • Code style: imports ordered, good names, simple structure, etc
  • Comments saying why design decisions were made
  • Go unit tests, with comments saying what you're testing
  • Integration tests, with comments saying what you're testing
  • doc.go added or updated in changed packages

QA steps

juju bootstrap --constraints "a=b" --constraints "b=c"
juju bootstrap --bootstrap-constraints "a=b" --bootstrap-constraints "b=c"
juju deploy indico --constraints "a=b" --constraints "b=c"
juju add-machine --constraints "a=b" --constraints "b=c"
juju enable-ha --constraints "a=b" --constraints "b=c"

Documentation changes

Links

Launchpad bug: https://bugs.launchpad.net/juju/+bug/2049439
Jira card: JUJU-

TODO

  • the "--bootstrap-constraints" flag on the bootstrap command.
  • the "--constraints" flag on the deploy command.
  • the "--constraints" flag on the add-machine command.
  • the "--constraints" flag on the enable-ha command.

@jujubot
Copy link
Collaborator

jujubot commented Apr 3, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

7 similar comments
@jujubot
Copy link
Collaborator

jujubot commented Apr 3, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Apr 3, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Apr 3, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Apr 3, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Apr 3, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Apr 3, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Apr 3, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@amandahla amandahla mentioned this pull request Apr 3, 2024
Closed
5 tasks
@hpidcock hpidcock added the 3.3 label Apr 3, 2024
@hpidcock
Copy link
Member

hpidcock commented Apr 3, 2024

@amandahla thank-you for the contribution. Could you please re-base this to 3.5 as this is a feature rather than a true bug fix.

Also I think we should strive to have consistency here, so if you could also please implement the same logic for:

  • the "--bootstrap-constraints" flag on the bootstrap command.
  • the "--constraints" flag on the deploy command.
  • the "--constraints" flag on the add-machine command.
  • the "--constraints" flag on the enable-ha command.

Please let me know if you have any questions.

@amandahla
Copy link
Contributor Author

@amandahla thank-you for the contribution. Could you please re-base this to 3.5 as this is a feature rather than a true bug fix.

Also I think we should strive to have consistency here, so if you could also please implement the same logic for:

  • the "--bootstrap-constraints" flag on the bootstrap command.
  • the "--constraints" flag on the deploy command.
  • the "--constraints" flag on the add-machine command.
  • the "--constraints" flag on the enable-ha command.

Please let me know if you have any questions.

Hi, could you please confirm? I was asked to target 3.3 here:
#17123 (review)

@hpidcock
Copy link
Member

@amandahla thank-you for the contribution. Could you please re-base this to 3.5 as this is a feature rather than a true bug fix.
Also I think we should strive to have consistency here, so if you could also please implement the same logic for:

  • the "--bootstrap-constraints" flag on the bootstrap command.
  • the "--constraints" flag on the deploy command.
  • the "--constraints" flag on the add-machine command.
  • the "--constraints" flag on the enable-ha command.

Please let me know if you have any questions.

Hi, could you please confirm? I was asked to target 3.3 here: #17123 (review)

This will need to target 3.6 now, since 3.5 has gone to rc1. Since this is a feature, we don't typically land these in released versions. It will also need to address inconsistency it introduces between constraints flags, otherwise we can't land it.

We are happy for this feature to go ahead as it makes sense from a design perspective, just need to iron out the issues around bootstrap/deploy/add-machine/enable-ha uses of constraints.

@hpidcock hpidcock changed the base branch from 3.3 to 3.6 April 24, 2024 00:49
@hpidcock hpidcock added 3.6 and removed 3.3 labels Apr 24, 2024
@amandahla amandahla changed the title Allow multiple constraints flag in bootstrap command Allow multiple constraints flag Jun 3, 2024
@amandahla
Copy link
Contributor Author

Thanks @hpidcock , the bootstrap/deploy/add-machine/enable-ha commands were updated now to support multiple constraints.

@hpidcock
Copy link
Member

hpidcock commented Jun 3, 2024

/build

hpidcock
hpidcock approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@hpidcock hpidcock left a comment

Choose a reason for hiding this comment

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

Thank-you for the patch. This is a excellent improvement.

@hpidcock
Copy link
Member

hpidcock commented Jun 5, 2024

/merge

@jujubot jujubot merged commit f11d8a3 into juju:3.6 Jun 5, 2024
19 of 21 checks passed
@jack-w-shaw jack-w-shaw mentioned this pull request Jun 6, 2024
Merged
jujubot added a commit that referenced this pull request Jun 7, 2024
@jujubot
Merge pull request #17479 from jack-w-shaw/3.6-into-main
40de207 
#17479

Forward merge:
- #17348
- #17413
- #17423
- #17436
- #17336
- #17422
- #17446
- #17448
- #17444
- #17453
- #17440
- #17458
- #17454
- #17461
- #17462
- #17463
- #17465
- #17469
- #17144
- #17179
- #17464
- #17476
- #17477

Conflicts:
- cmd/juju/application/bundle/bundle.go
- cmd/juju/application/bundle/bundle_test.go
- cmd/juju/application/deploy.go
- cmd/juju/application/deploy_test.go
- cmd/juju/commands/bootstrap.go
- cmd/juju/machine/add.go
- cmd/jujud-controller/agent/machine.go
- cmd/jujud/agent/machine.go
- go.mod
- go.sum
- internal/bundle/changes/changes.go
- internal/container/utils.go
- internal/container/utils_test.go
- internal/provider/azure/environ.go
- internal/provider/azure/internal/errorutils/errors_test.go
- internal/provider/azure/internal/imageutils/images.go
- internal/provider/azure/internal/imageutils/images_test.go
- internal/worker/containerbroker/broker_test.go
- internal/worker/uniter/runner/context/context.go
- internal/worker/uniter/runner/context/contextfactory.go
- internal/worker/uniter/runner/context/export_test.go
- internal/worker/uniter/runner/jujuc/relation-ids.go
- tests/includes/juju.sh
- tests/main.sh
- tests/suites/deploy/os.sh

Most conflicts were trivial to solve. Azure conflicts took a bit of thought. They emerged from [my PR](#17440)

The conflicts to `internal/worker/uniter/runner`, emerging from [this PR](#17464), took some thought, since the structure of tests had changed to mocks. The conflicts with the production code here, however, was trivial to merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hpidcock hpidcock hpidcock approved these changes

Assignees
No one assigned
Labels
3.6
Projects
None yet
Milestone
No milestone
3 participants
@amandahla @jujubot @hpidcock