diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2a0dd28582f..9e872595e79 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -301,6 +301,38 @@ with an environment variable: JUJU_NOTEST_MONGOJS=1 go test github.com/juju/juju/... ``` +Conventional commits +-------------------- + +Once you have written some code and have tested the changes, the next step is to +`git commit` it. For commit messages Juju follows [conventional commits guidelines](doc/conventional-commits.md). +In short the commits should be of the following form: +``` +(optional ): + +[optional body] + +[optional footer(s)] +``` +- **Type:** The type describes the kind of change (e.g., feat, fix, docs, style, + refactor, test, chore). +- **Scope:** The scope indicates the part of the codebase affected (e.g., model, + api, cli). +- **Description:** The description briefly describes the change. It should not + end in any punctuation. +- **Body:** The body should be a detailed explanation of the change. It should + specify what has been changed and why and include descriptions of the + behaviour before and after. +- **Footer:** The footer includes information about breaking changes, issues + closed, etc. + +The type, scope and description should all begin with a lower case letter. None +of the lines can exceed 100 characters in length. + +There is a CI action on the Juju GitHub repository that will flag any syntactic +issues with your commit messages. The action is required to pass before code can +be merged so make sure to take a look once your PR is up. + Pushing ------- @@ -318,6 +350,9 @@ Go to the web page (https://github.com/$YOUR_GITHUB_USERNAME/juju) and hit the This creates a numbered pull request on the github site, where members of the Juju project can see and comment on the changes. +The title of the PR should match the form of the title of a conventional commit. +This can be the title of the most significant commit in the PR. + Make sure to add a clear description of why and what has been changed, and include the Launchpad bug number if one exists. diff --git a/cmd/modelcmd/base.go b/cmd/modelcmd/base.go index 9f982d0f177..e33a7f58403 100644 --- a/cmd/modelcmd/base.go +++ b/cmd/modelcmd/base.go @@ -226,16 +226,12 @@ func processAccountDetails(accountDetails *jujuclient.AccountDetails) *jujuclien } else { u := names.NewUserTag(accountDetails.User) if !u.IsLocal() { - if len(accountDetails.Macaroons) == 0 { - accountDetails = &jujuclient.AccountDetails{} - } else { - // If the account has macaroon set, use those to login - // to avoid an unnecessary auth round trip. - // Used for embedded commands. - accountDetails = &jujuclient.AccountDetails{ - User: u.Id(), - Macaroons: accountDetails.Macaroons, - } + // If the account has macaroon set, use those to login + // to avoid an unnecessary auth round trip. + // Used for embedded commands. + accountDetails = &jujuclient.AccountDetails{ + User: u.Id(), + Macaroons: accountDetails.Macaroons, } } } diff --git a/cmd/modelcmd/base_test.go b/cmd/modelcmd/base_test.go index 41ae0c80056..ec5e3214ac0 100644 --- a/cmd/modelcmd/base_test.go +++ b/cmd/modelcmd/base_test.go @@ -17,6 +17,7 @@ import ( cookiejar "github.com/juju/persistent-cookiejar" "github.com/juju/testing" jc "github.com/juju/testing/checkers" + "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" "gopkg.in/macaroon-bakery.v2/httpbakery" "gopkg.in/macaroon.v2" @@ -25,6 +26,7 @@ import ( apitesting "github.com/juju/juju/api/testing" "github.com/juju/juju/cloud" "github.com/juju/juju/cmd/modelcmd" + "github.com/juju/juju/cmd/modelcmd/mocks" "github.com/juju/juju/core/model" "github.com/juju/juju/core/network" "github.com/juju/juju/environs" @@ -190,6 +192,42 @@ To access it run 'juju switch bar:admin/badmodel'.`, } } +func (s *BaseCommandSuite) setupMocks(c *gc.C) *gomock.Controller { + ctrl := gomock.NewController(c) + return ctrl +} + +func (s *BaseCommandSuite) TestNewAPIRootExternalUser(c *gc.C) { + ctrl := s.setupMocks(c) + conn := mocks.NewMockConnection(ctrl) + apiOpen := func(info *api.Info, opts api.DialOpts) (api.Connection, error) { + return conn, nil + } + externalName := "alastair@external" + conn.EXPECT().AuthTag().Return(names.NewUserTag(externalName)).MinTimes(1) + conn.EXPECT().APIHostPorts() + conn.EXPECT().ServerVersion() + conn.EXPECT().Addr() + conn.EXPECT().IPAddr() + conn.EXPECT().PublicDNSName() + conn.EXPECT().ControllerAccess().MinTimes(1) + + s.store.Accounts["foo"] = jujuclient.AccountDetails{ + User: externalName, + } + + baseCmd := new(modelcmd.ModelCommandBase) + baseCmd.SetClientStore(s.store) + baseCmd.SetAPIOpen(apiOpen) + modelcmd.InitContexts(&cmd.Context{Stderr: io.Discard}, baseCmd) + modelcmd.SetRunStarted(baseCmd) + + c.Assert(baseCmd.SetModelIdentifier("foo:admin/badmodel", false), jc.ErrorIsNil) + + _, err := baseCmd.NewAPIRoot() + c.Assert(err, jc.ErrorIsNil) +} + type NewGetBootstrapConfigParamsFuncSuite struct { testing.IsolationSuite } @@ -462,7 +500,9 @@ func (s *BaseCommandSuite) TestProcessAccountDetails(c *gc.C) { input: jujuclient.AccountDetails{ User: names.NewUserTag("alice@wonderland.canonical.com").String(), }, - expectedOutput: jujuclient.AccountDetails{}, + expectedOutput: jujuclient.AccountDetails{ + User: names.NewUserTag("alice@wonderland.canonical.com").String(), + }, }} for i, test := range tests { c.Logf("running test case %d", i) diff --git a/cmd/modelcmd/mocks/api_mock.go b/cmd/modelcmd/mocks/api_mock.go new file mode 100644 index 00000000000..4d8fe92c87c --- /dev/null +++ b/cmd/modelcmd/mocks/api_mock.go @@ -0,0 +1,391 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/api (interfaces: Connection) +// +// Generated by this command: +// +// mockgen -package mocks -destination mocks/api_mock.go github.com/juju/juju/api Connection +// + +// Package mocks is a generated GoMock package. +package mocks + +import ( + context "context" + http "net/http" + url "net/url" + reflect "reflect" + + base "github.com/juju/juju/api/base" + network "github.com/juju/juju/core/network" + proxy "github.com/juju/juju/proxy" + names "github.com/juju/names/v5" + version "github.com/juju/version/v2" + gomock "go.uber.org/mock/gomock" + httprequest "gopkg.in/httprequest.v1" + macaroon "gopkg.in/macaroon.v2" +) + +// MockConnection is a mock of Connection interface. +type MockConnection struct { + ctrl *gomock.Controller + recorder *MockConnectionMockRecorder +} + +// MockConnectionMockRecorder is the mock recorder for MockConnection. +type MockConnectionMockRecorder struct { + mock *MockConnection +} + +// NewMockConnection creates a new mock instance. +func NewMockConnection(ctrl *gomock.Controller) *MockConnection { + mock := &MockConnection{ctrl: ctrl} + mock.recorder = &MockConnectionMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockConnection) EXPECT() *MockConnectionMockRecorder { + return m.recorder +} + +// APICall mocks base method. +func (m *MockConnection) APICall(arg0 string, arg1 int, arg2, arg3 string, arg4, arg5 any) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "APICall", arg0, arg1, arg2, arg3, arg4, arg5) + ret0, _ := ret[0].(error) + return ret0 +} + +// APICall indicates an expected call of APICall. +func (mr *MockConnectionMockRecorder) APICall(arg0, arg1, arg2, arg3, arg4, arg5 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "APICall", reflect.TypeOf((*MockConnection)(nil).APICall), arg0, arg1, arg2, arg3, arg4, arg5) +} + +// APIHostPorts mocks base method. +func (m *MockConnection) APIHostPorts() []network.MachineHostPorts { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "APIHostPorts") + ret0, _ := ret[0].([]network.MachineHostPorts) + return ret0 +} + +// APIHostPorts indicates an expected call of APIHostPorts. +func (mr *MockConnectionMockRecorder) APIHostPorts() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "APIHostPorts", reflect.TypeOf((*MockConnection)(nil).APIHostPorts)) +} + +// Addr mocks base method. +func (m *MockConnection) Addr() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Addr") + ret0, _ := ret[0].(string) + return ret0 +} + +// Addr indicates an expected call of Addr. +func (mr *MockConnectionMockRecorder) Addr() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Addr", reflect.TypeOf((*MockConnection)(nil).Addr)) +} + +// AuthTag mocks base method. +func (m *MockConnection) AuthTag() names.Tag { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AuthTag") + ret0, _ := ret[0].(names.Tag) + return ret0 +} + +// AuthTag indicates an expected call of AuthTag. +func (mr *MockConnectionMockRecorder) AuthTag() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AuthTag", reflect.TypeOf((*MockConnection)(nil).AuthTag)) +} + +// BakeryClient mocks base method. +func (m *MockConnection) BakeryClient() base.MacaroonDischarger { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "BakeryClient") + ret0, _ := ret[0].(base.MacaroonDischarger) + return ret0 +} + +// BakeryClient indicates an expected call of BakeryClient. +func (mr *MockConnectionMockRecorder) BakeryClient() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BakeryClient", reflect.TypeOf((*MockConnection)(nil).BakeryClient)) +} + +// BestFacadeVersion mocks base method. +func (m *MockConnection) BestFacadeVersion(arg0 string) int { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "BestFacadeVersion", arg0) + ret0, _ := ret[0].(int) + return ret0 +} + +// BestFacadeVersion indicates an expected call of BestFacadeVersion. +func (mr *MockConnectionMockRecorder) BestFacadeVersion(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BestFacadeVersion", reflect.TypeOf((*MockConnection)(nil).BestFacadeVersion), arg0) +} + +// Broken mocks base method. +func (m *MockConnection) Broken() <-chan struct{} { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Broken") + ret0, _ := ret[0].(<-chan struct{}) + return ret0 +} + +// Broken indicates an expected call of Broken. +func (mr *MockConnectionMockRecorder) Broken() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Broken", reflect.TypeOf((*MockConnection)(nil).Broken)) +} + +// Close mocks base method. +func (m *MockConnection) Close() error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Close") + ret0, _ := ret[0].(error) + return ret0 +} + +// Close indicates an expected call of Close. +func (mr *MockConnectionMockRecorder) Close() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockConnection)(nil).Close)) +} + +// ConnectControllerStream mocks base method. +func (m *MockConnection) ConnectControllerStream(arg0 string, arg1 url.Values, arg2 http.Header) (base.Stream, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ConnectControllerStream", arg0, arg1, arg2) + ret0, _ := ret[0].(base.Stream) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ConnectControllerStream indicates an expected call of ConnectControllerStream. +func (mr *MockConnectionMockRecorder) ConnectControllerStream(arg0, arg1, arg2 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ConnectControllerStream", reflect.TypeOf((*MockConnection)(nil).ConnectControllerStream), arg0, arg1, arg2) +} + +// ConnectStream mocks base method. +func (m *MockConnection) ConnectStream(arg0 string, arg1 url.Values) (base.Stream, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ConnectStream", arg0, arg1) + ret0, _ := ret[0].(base.Stream) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ConnectStream indicates an expected call of ConnectStream. +func (mr *MockConnectionMockRecorder) ConnectStream(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ConnectStream", reflect.TypeOf((*MockConnection)(nil).ConnectStream), arg0, arg1) +} + +// Context mocks base method. +func (m *MockConnection) Context() context.Context { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Context") + ret0, _ := ret[0].(context.Context) + return ret0 +} + +// Context indicates an expected call of Context. +func (mr *MockConnectionMockRecorder) Context() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Context", reflect.TypeOf((*MockConnection)(nil).Context)) +} + +// ControllerAccess mocks base method. +func (m *MockConnection) ControllerAccess() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ControllerAccess") + ret0, _ := ret[0].(string) + return ret0 +} + +// ControllerAccess indicates an expected call of ControllerAccess. +func (mr *MockConnectionMockRecorder) ControllerAccess() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControllerAccess", reflect.TypeOf((*MockConnection)(nil).ControllerAccess)) +} + +// ControllerTag mocks base method. +func (m *MockConnection) ControllerTag() names.ControllerTag { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ControllerTag") + ret0, _ := ret[0].(names.ControllerTag) + return ret0 +} + +// ControllerTag indicates an expected call of ControllerTag. +func (mr *MockConnectionMockRecorder) ControllerTag() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControllerTag", reflect.TypeOf((*MockConnection)(nil).ControllerTag)) +} + +// CookieURL mocks base method. +func (m *MockConnection) CookieURL() *url.URL { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CookieURL") + ret0, _ := ret[0].(*url.URL) + return ret0 +} + +// CookieURL indicates an expected call of CookieURL. +func (mr *MockConnectionMockRecorder) CookieURL() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CookieURL", reflect.TypeOf((*MockConnection)(nil).CookieURL)) +} + +// HTTPClient mocks base method. +func (m *MockConnection) HTTPClient() (*httprequest.Client, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HTTPClient") + ret0, _ := ret[0].(*httprequest.Client) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HTTPClient indicates an expected call of HTTPClient. +func (mr *MockConnectionMockRecorder) HTTPClient() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HTTPClient", reflect.TypeOf((*MockConnection)(nil).HTTPClient)) +} + +// IPAddr mocks base method. +func (m *MockConnection) IPAddr() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IPAddr") + ret0, _ := ret[0].(string) + return ret0 +} + +// IPAddr indicates an expected call of IPAddr. +func (mr *MockConnectionMockRecorder) IPAddr() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IPAddr", reflect.TypeOf((*MockConnection)(nil).IPAddr)) +} + +// IsBroken mocks base method. +func (m *MockConnection) IsBroken() bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsBroken") + ret0, _ := ret[0].(bool) + return ret0 +} + +// IsBroken indicates an expected call of IsBroken. +func (mr *MockConnectionMockRecorder) IsBroken() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsBroken", reflect.TypeOf((*MockConnection)(nil).IsBroken)) +} + +// IsProxied mocks base method. +func (m *MockConnection) IsProxied() bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsProxied") + ret0, _ := ret[0].(bool) + return ret0 +} + +// IsProxied indicates an expected call of IsProxied. +func (mr *MockConnectionMockRecorder) IsProxied() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsProxied", reflect.TypeOf((*MockConnection)(nil).IsProxied)) +} + +// Login mocks base method. +func (m *MockConnection) Login(arg0 names.Tag, arg1, arg2 string, arg3 []macaroon.Slice) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Login", arg0, arg1, arg2, arg3) + ret0, _ := ret[0].(error) + return ret0 +} + +// Login indicates an expected call of Login. +func (mr *MockConnectionMockRecorder) Login(arg0, arg1, arg2, arg3 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Login", reflect.TypeOf((*MockConnection)(nil).Login), arg0, arg1, arg2, arg3) +} + +// ModelTag mocks base method. +func (m *MockConnection) ModelTag() (names.ModelTag, bool) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ModelTag") + ret0, _ := ret[0].(names.ModelTag) + ret1, _ := ret[1].(bool) + return ret0, ret1 +} + +// ModelTag indicates an expected call of ModelTag. +func (mr *MockConnectionMockRecorder) ModelTag() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ModelTag", reflect.TypeOf((*MockConnection)(nil).ModelTag)) +} + +// Proxy mocks base method. +func (m *MockConnection) Proxy() proxy.Proxier { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Proxy") + ret0, _ := ret[0].(proxy.Proxier) + return ret0 +} + +// Proxy indicates an expected call of Proxy. +func (mr *MockConnectionMockRecorder) Proxy() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Proxy", reflect.TypeOf((*MockConnection)(nil).Proxy)) +} + +// PublicDNSName mocks base method. +func (m *MockConnection) PublicDNSName() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "PublicDNSName") + ret0, _ := ret[0].(string) + return ret0 +} + +// PublicDNSName indicates an expected call of PublicDNSName. +func (mr *MockConnectionMockRecorder) PublicDNSName() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PublicDNSName", reflect.TypeOf((*MockConnection)(nil).PublicDNSName)) +} + +// RootHTTPClient mocks base method. +func (m *MockConnection) RootHTTPClient() (*httprequest.Client, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RootHTTPClient") + ret0, _ := ret[0].(*httprequest.Client) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// RootHTTPClient indicates an expected call of RootHTTPClient. +func (mr *MockConnectionMockRecorder) RootHTTPClient() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RootHTTPClient", reflect.TypeOf((*MockConnection)(nil).RootHTTPClient)) +} + +// ServerVersion mocks base method. +func (m *MockConnection) ServerVersion() (version.Number, bool) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ServerVersion") + ret0, _ := ret[0].(version.Number) + ret1, _ := ret[1].(bool) + return ret0, ret1 +} + +// ServerVersion indicates an expected call of ServerVersion. +func (mr *MockConnectionMockRecorder) ServerVersion() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ServerVersion", reflect.TypeOf((*MockConnection)(nil).ServerVersion)) +} diff --git a/cmd/modelcmd/package_test.go b/cmd/modelcmd/package_test.go index 536f95b83b2..8c7b377ab9e 100644 --- a/cmd/modelcmd/package_test.go +++ b/cmd/modelcmd/package_test.go @@ -10,6 +10,7 @@ import ( ) //go:generate go run go.uber.org/mock/mockgen -package mocks -destination mocks/modelconfig_mock.go github.com/juju/juju/cmd/modelcmd ModelConfigAPI +//go:generate go run go.uber.org/mock/mockgen -package mocks -destination mocks/api_mock.go github.com/juju/juju/api Connection func Test(t *testing.T) { coretesting.MgoTestPackage(t) diff --git a/doc/conventional-commits.md b/doc/conventional-commits.md new file mode 100644 index 00000000000..3bc01b2ac1b --- /dev/null +++ b/doc/conventional-commits.md @@ -0,0 +1,153 @@ +The [Conventional Commits standard](https://www.conventionalcommits.org/en/v1.0.0/) is adopted for the Juju project with the +following structure: + +``` +(optional ): + +[optional body] + +[optional footer(s)] +``` + +- **type**: Describes the kind of change (e.g., feat, fix, docs, style, refactor, test, chore). + +|Type|Kind of change|Description| +|---|---|---| +|feat|Features|A new feature| +|fix|Bug fixes|A bug fix| +|docs|Documentation|Documentation only changes| +|style|Styles|Changes that do not affect the meaning of the code| +|refactor|Code refactoring|A code change that neither fixes a bug nor adds a feature| +|perf|Performance improvements|A code change that improves performance| +|test|Tests|Adding missing tests or correcting existing tests| +|build|Builds|Changes that affect the build system or external dependencies| +|ci|Continuous integration|Changes to our CI configuration files and scripts| +|chore|Chores|Necessary yet mundane/trivial changes (updating dependencies, merging through …)| +|revert|Reverts|Reverts a previous commit| + +- **scope**: A scope indicating the part of the codebase affected (e.g., model, api, cli). + - Can be Optional is it’s hard to define the scope +- **description**: A brief summary of the change. + - Must be provided after the BREAKING CHANGE:, describing what has changed about the API, e.g., BREAKING CHANGE: environment variables now take precedence over config files. + - Should use lowercase. + - Should not end in any punctuation. +- **body**: Detailed explanation of the change. + - Can be Optional for small/trivial `fix`, but NOT for other types. + - Could consist of several paragraphs. + - Explanation of change should details what was before, and what is after, and avoid contextual terms like 'now' + - Good body should be on the form: + * Before this commit `` + * After this commit `` +- **footer**: (Optional) Information about breaking changes, issues closed, etc. + - A commit can contain a footer which must consist of one or more [Git trailers](https://git-scm.com/docs/git-interpret-trailers). + - The footer must start at the first occurrence of a blank line, followed by a Git trailer. + - Each trailer must start on its own line, using the format ``. + - The trailer `` must be either BREAKING CHANGE or be one or more words, grouped by hyphens (e.g. Co-Authored-By, fixes, etc.). + - The trailer `` must be one of `:` or `#`, supporting both `Co-Authored-By: Thomas Miller ` and `Fixes #999`. + - The trailer `` must be present and can contain any characters, either on a single line, split over multiple lines, or split over multiple paragraphs. + + +Here is the reference example of a conventional commit message: + +``` +feat(api): add user authentication feature + +# scope: Indicates the part of the codebase affected +# Here, 'api' is specified as the scope + +# description: A brief summary of the change in lower-case +# "add user authentication feature" summarizes the change concisely + +This commit adds user authentication to the API. Users can now sign up, +log in, and log out. Passwords are hashed using bcrypt. Token-based  +authentication is implemented using JWT. + +# body: Detailed explanation of the change +# A more detailed explanation of what has been changed and why + +BREAKING CHANGE: The user authentication changes the login endpoint +from `/api/login` to `/api/v1/login`. All previous tokens are now invalid, +and users will need to reauthenticate. + +# footer: Information about breaking changes +# Here, the footer indicates a breaking change, describing what has changed +# about the API and how it impacts users + +Authored-By: Thomas Miller <[thomas.miller@canonical.com](mailto:thomas.miller@canonical.com)> + +Fixes #123 + +# footer: Issues closed or other information +# This footer references to and author and an issue that this commit closes. +``` + +The PR title, where a conventional commit is introduced, should be the same format as a first line of a commit description: `(optional ): `. In case this commit is core/base/most significant for this PR. + +### Footer and Multi-paragraph Body +### Juju-Specific Scopes + +Define specific scopes relevant to the Juju project to enhance clarity. Examples include: + +- model: Logical grouping of applications, representing an environment. +- controller: Manages and oversees multiple models, providing centralized management. +- charm: Encapsulated operational code for applications, defining how they are deployed and managed. +- application: Deployed instance of a charm, representing a running service. +- unit: Individual instance of an application, running on a machine. +- relation: Connection between different applications, enabling them to communicate and share data. +- bundle: Collection of applications and relations, defining a complex deployment setup. +- action: Scripted operation on an application or unit, allowing for specific tasks to be performed. +- machine: Physical or virtual host running units, part of the infrastructure. +- storage: Persistent data storage for units, ensuring data remains available across restarts. +- offer: Exposed application endpoint to other models, allowing cross-model relations. +- endpoint: Specific interaction point for a relation within an application. +- constraint: Resource specifications for machines and applications, such as CPU and memory requirements. +- user: Individual with access to the Juju environment, assigned roles and permissions. +- space: Logical network segmentation within models, aiding in network isolation and security. +- subnet: IP address range within a space, used for assigning IPs to machines and units. +- backup: Process of saving the state of a model or controller for recovery purposes. +- migration: Moving a model or application from one controller to another. +- resource: Files or data required by charms during deployment and operation. +- upgrade: Process of updating charms, applications, or Juju itself to newer versions. +- cloud: Infrastructure provider where models and applications are deployed, such as AWS, Azure, or OpenStack. +- credential: Authentication details for accessing cloud providers and resources. + +This list just illustrates the idea and suggestion of which single words should be used for scope definition. It is NOT complete and is NOT limited by these scopes. + +### Other Examples +``` +feat(networking): add juju networking support for all provider + + +``` +--- +``` +feat!: remove ticket list endpoint + +refers to JIRA-666 + +BREAKING CHANGES: ticket enpoints no longer supports list all entites. +``` +--- +``` +fix(migration): fix the universal migration to any major/minor version in future + + +``` +--- +``` +docs: add support for multi-version + tips windows + + +``` +--- +``` +refactor(cmr): remove all uclear and understandle code from CMR implementation + + +``` +--- +``` +fix(backup): fix backup db state issue + + +``` \ No newline at end of file diff --git a/state/cleanup.go b/state/cleanup.go index 6a3459844a9..a3f48614d93 100644 --- a/state/cleanup.go +++ b/state/cleanup.go @@ -46,6 +46,7 @@ const ( cleanupDyingMachine cleanupKind = "dyingMachine" cleanupForceDestroyedMachine cleanupKind = "machine" cleanupForceRemoveMachine cleanupKind = "forceRemoveMachine" + cleanupEvacuateMachine cleanupKind = "evacuateMachine" cleanupAttachmentsForDyingStorage cleanupKind = "storageAttachments" cleanupAttachmentsForDyingVolume cleanupKind = "volumeAttachments" cleanupAttachmentsForDyingFilesystem cleanupKind = "filesystemAttachments" @@ -221,6 +222,8 @@ func (st *State) Cleanup() (err error) { err = st.cleanupForceRemoveMachine(doc.Prefix, args) case cleanupAttachmentsForDyingStorage: err = st.cleanupAttachmentsForDyingStorage(doc.Prefix, args) + case cleanupEvacuateMachine: + err = st.cleanupEvacuateMachine(doc.Prefix, args) case cleanupAttachmentsForDyingVolume: err = st.cleanupAttachmentsForDyingVolume(doc.Prefix) case cleanupAttachmentsForDyingFilesystem: @@ -1436,6 +1439,62 @@ func (st *State) cleanupForceRemoveMachine(machineId string, cleanupArgs []bson. return machine.Remove() } +// cleanupEvacuateMachine is initiated by machine.Destroy() to gracefully remove units +// from the machine before then kicking off machine destroy. +func (st *State) cleanupEvacuateMachine(machineId string, cleanupArgs []bson.Raw) error { + if len(cleanupArgs) > 0 { + return errors.Errorf("expected no arguments, got %d", len(cleanupArgs)) + } + + machine, err := st.Machine(machineId) + if errors.IsNotFound(err) { + return nil + } else if err != nil { + return errors.Trace(err) + } + if machine.Life() != Alive { + return nil + } + + units, err := machine.Units() + if err != nil { + return errors.Trace(err) + } + + if len(units) == 0 { + if err := machine.advanceLifecycle(Dying, false, false, 0); err != nil { + return errors.Trace(err) + } + return nil + } + + buildTxn := func(attempt int) ([]txn.Op, error) { + if attempt > 0 { + units, err = machine.Units() + if err != nil { + return nil, errors.Trace(err) + } + } + var ops []txn.Op + for _, unit := range units { + destroyOp := unit.DestroyOperation() + op, err := destroyOp.Build(attempt) + if err != nil && !errors.Is(err, jujutxn.ErrNoOperations) { + return nil, errors.Trace(err) + } + ops = append(ops, op...) + } + return ops, nil + } + + err = st.db().Run(buildTxn) + if err != nil { + return errors.Trace(err) + } + + return errors.Errorf("waiting for units to be removed from %s", machineId) +} + // cleanupContainers recursively calls cleanupForceDestroyedMachine on the supplied // machine's containers, and removes them from state entirely. func (st *State) cleanupContainers(machine *Machine, maxWait time.Duration) error { diff --git a/state/cleanup_test.go b/state/cleanup_test.go index f2c085a679e..9a3094358a0 100644 --- a/state/cleanup_test.go +++ b/state/cleanup_test.go @@ -6,6 +6,7 @@ package state_test import ( "bytes" "sort" + "strconv" "time" "github.com/juju/charm/v12" @@ -473,6 +474,56 @@ func (s *CleanupSuite) TestDestroyControllerMachineErrors(c *gc.C) { assertLife(c, manager, state.Alive) } +func (s *CleanupSuite) TestDestroyControllerMachineHAWithControllerCharm(c *gc.C) { + cons := constraints.Value{ + Mem: newUint64(100), + } + changes, err := s.State.EnableHA(3, cons, state.UbuntuBase("12.04"), nil) + c.Assert(err, jc.ErrorIsNil) + c.Assert(changes.Added, gc.HasLen, 3) + + ch := s.AddTestingCharmWithSeries(c, "juju-controller", "") + app := s.AddTestingApplicationForBase(c, state.UbuntuBase("12.04"), "controller", ch) + + var machines []*state.Machine + var units []*state.Unit + for i := 0; i < 3; i++ { + m, err := s.State.Machine(strconv.Itoa(i)) + c.Assert(err, jc.ErrorIsNil) + c.Assert(m.Jobs(), gc.DeepEquals, []state.MachineJob{ + state.JobHostUnits, + state.JobManageModel, + }) + + if i == 0 { + node, err := s.State.ControllerNode(m.Id()) + c.Assert(err, jc.ErrorIsNil) + node.SetHasVote(true) + } + + u, err := app.AddUnit(state.AddUnitParams{}) + c.Assert(err, jc.ErrorIsNil) + err = u.SetCharmURL(ch.URL()) + c.Assert(err, jc.ErrorIsNil) + err = u.AssignToMachine(m) + c.Assert(err, jc.ErrorIsNil) + + machines = append(machines, m) + units = append(units, u) + } + + for _, m := range machines { + assertLife(c, m, state.Alive) + } + for _, u := range units { + assertLife(c, u, state.Alive) + } + + s.assertDoesNotNeedCleanup(c) + err = machines[2].Destroy() + c.Assert(err, gc.ErrorMatches, ".* has unit .* assigned") +} + const dontWait = time.Duration(0) func (s *CleanupSuite) TestCleanupForceDestroyedMachineUnit(c *gc.C) { @@ -534,7 +585,6 @@ func (s *CleanupSuite) TestCleanupForceDestroyedControllerMachine(c *gc.C) { // The machine should no longer want the vote, should be forced to not have the vote, and forced to not be a // controller member anymore c.Assert(machine.Refresh(), jc.ErrorIsNil) - c.Check(machine.Life(), gc.Equals, state.Dying) node, err = s.State.ControllerNode(machine.Id()) c.Assert(err, jc.ErrorIsNil) c.Check(node.WantsVote(), jc.IsFalse) @@ -548,21 +598,15 @@ func (s *CleanupSuite) TestCleanupForceDestroyedControllerMachine(c *gc.C) { s.assertCleanupRuns(c) c.Assert(node.SetHasVote(false), jc.ErrorIsNil) // However, if we remove the vote, it can be cleaned up. - // ForceDestroy sets up a cleanupForceDestroyedMachine, which - // calls advanceLifecycle(Dead) which sets up a - // cleanupDyingMachine, which in turn creates a delayed - // cleanupForceRemoveMachine. - // Run the first two. - s.assertCleanupCountDirty(c, 2) - // After we've run the cleanup for the controller machine, the machine should be dead, and it should not be + // ForceDestroy sets up a cleanupEvacuateMachine, which will not + // add any other cleanup ops. + // After we've run the cleanup for the controller machine, the machine should be dying, and it should not be // present in the other documents. - assertLife(c, machine, state.Dead) + assertLife(c, machine, state.Dying) controllerIds, err = s.State.ControllerIds() c.Assert(err, jc.ErrorIsNil) sort.Strings(controllerIds) sort.Strings(changes.Added) - // Only the machines that were added should still be part of the controller - c.Check(controllerIds, gc.DeepEquals, changes.Added) } func (s *CleanupSuite) TestCleanupForceDestroyMachineCleansStorageAttachments(c *gc.C) { diff --git a/state/enableha_test.go b/state/enableha_test.go index 49ba19bcb1d..1413f6c4dd2 100644 --- a/state/enableha_test.go +++ b/state/enableha_test.go @@ -525,8 +525,9 @@ func (s *EnableHASuite) TestForceDestroyFromHA(c *gc.C) { err = m0.ForceDestroy(dontWait) c.Assert(err, jc.ErrorIsNil) c.Assert(m0.Refresh(), jc.ErrorIsNil) - // Could this actually get all the way to Dead? - c.Check(m0.Life(), gc.Equals, state.Dying) + // Force remove of controller machines will first clean up units and + // after that it will pass the machine life to Dying. + c.Check(m0.Life(), gc.Equals, state.Alive) c.Assert(node.Refresh(), jc.ErrorIsNil) c.Assert(node.WantsVote(), jc.IsFalse) } diff --git a/state/machine.go b/state/machine.go index a9e2d367009..7b0910181d8 100644 --- a/state/machine.go +++ b/state/machine.go @@ -639,31 +639,25 @@ func (m *Machine) forceDestroyOps(maxWait time.Duration) ([]txn.Op, error) { if len(controllerIds) <= 1 { return nil, errors.Errorf("controller %s is the only controller", m.Id()) } - // We set the machine to Dying if it isn't already dead. - var machineOp txn.Op - if m.Life() < Dead { - // Make sure we don't want the vote, and we are queued to be Dying. - // Since we are force deleting, life assert should be current machine's life. - machineOp = txn.Op{ - C: machinesC, - Id: m.doc.DocID, - Assert: bson.D{{"life", bson.D{{"$in", []Life{Alive, Dying}}}}}, - Update: bson.D{{"$set", bson.D{{"life", Dying}}}}, - } - } - controllerOp := txn.Op{ - C: controllersC, - Id: modelGlobalKey, - Assert: bson.D{{"controller-ids", controllerIds}}, - } - // Note that ForceDestroy does *not* cleanup the replicaset, so it might cause problems. - // However, we're letting the user handle times when the machine agent isn't running, etc. - // We may need to update the peergrouper for this. return []txn.Op{ - machineOp, - controllerOp, + { + C: machinesC, + Id: m.doc.DocID, + Assert: bson.D{ + {"life", Alive}, + advanceLifecycleUnitAsserts(m.doc.Principals), + }, + // To prevent race conditions, we remove the ability for new + // units to be deployed to the machine. + Update: bson.D{{"$pull", bson.D{{"jobs", JobHostUnits}}}}, + }, + { + C: controllersC, + Id: modelGlobalKey, + Assert: bson.D{{"controller-ids", controllerIds}}, + }, setControllerWantsVoteOp(m.st, m.Id(), false), - newCleanupOp(cleanupForceDestroyedMachine, m.doc.Id, maxWait), + newCleanupOp(cleanupEvacuateMachine, m.doc.Id), }, nil } else { // Make sure the machine doesn't become a manager while we're destroying it diff --git a/state/machine_test.go b/state/machine_test.go index 2a544a8ad04..e19ffdb2c4b 100644 --- a/state/machine_test.go +++ b/state/machine_test.go @@ -374,7 +374,9 @@ func (s *MachineSuite) TestLifeJobManageModelWithControllerCharm(c *gc.C) { err = m2.Refresh() c.Assert(err, jc.ErrorIsNil) - c.Assert(m2.Life(), gc.Equals, state.Dying) + // Force remove of controller machines will first clean up units and + // after that it will pass the machine life to Dying. + c.Assert(m2.Life(), gc.Equals, state.Alive) cn2, err := s.State.ControllerNode(m2.Id()) c.Assert(err, jc.ErrorIsNil)