feat(supported-versions): add SupportedVersions facade method#1908
Conversation
SupportedVersions will return a list of supported Juju version JAAS allows
luci1900
left a comment
There was a problem hiding this comment.
Good idea.
We could also add a jaas command that calls this endpoint.
kian99
left a comment
There was a problem hiding this comment.
The only downside with this approach is that JIMM needs to be updated, released and deployed before a user will ever see new versions of Juju from the supported versions endpoint.
That decision seems like something we should resolve beforehand.
I've discussed it briefly in the standup after you were gone, i'll bring it up today again |
| // ContextualVersion is an optional version string that can be used to provide context | ||
| // for the supported versions response. | ||
| // For example, this could be used to indicate the version of the client making the request, | ||
| // allowing the server to tailor the supported versions response accordingly. |
There was a problem hiding this comment.
Isn't this too vague? We currently use it to set a minimum version, but the way this docstring is phrased seems like its too open-ended.
There was a problem hiding this comment.
i'm in favor of leaving the ContextualVersion in because we could change the filtering mechanism not to be minVersion. That's why it's vague, to avoid doing breaking changes to the facade if we change the filtering mechanism.
There was a problem hiding this comment.
It would still be a breaking change in behavior, even if the field doesn't change.
I strongly feel we should make the API clear and easy to use and if we identify that something must change, we should do a facade bump or make a new method/add a param for the new functionality.
There was a problem hiding this comment.
my vote goes to minVersion
There was a problem hiding this comment.
implemented with minVersion
alesstimec
left a comment
There was a problem hiding this comment.
looks good, once all comments are addressed
| defer cancel() | ||
|
|
||
| client := github.NewClient(nil) | ||
| minVersion := version.MustParse(minSupportedVersion) |
There was a problem hiding this comment.
we shouldn't be using MustParse in production code.. can't we just do:
minVersion := version.Number{
Major: x,
Minor: y,
Patch: z.
}
There was a problem hiding this comment.
in this case it's fine because minSupportedVersion is a const string. If this panics, it will panic in tests as well
| func TestSupportedVersions_MinVersion(t *testing.T) { | ||
| c := qt.New(t) | ||
|
|
||
| t.Run("filters to versions strictly greater than minVersion", func(t *testing.T) { |
There was a problem hiding this comment.
Minor
| t.Run("filters to versions strictly greater than minVersion", func(t *testing.T) { | |
| c.Run("filters to versions strictly greater than minVersion", func(c * qt.C) { |
That way you get a slightly different error trace if the test fails I believe.
| // MinVersion is an optional lower-bound version filter. | ||
| // When set, only versions strictly greater than MinVersion are included in the response. | ||
| // Versions equal to or below MinVersion are excluded. | ||
| MinVersion *string `json:"min-version,omitempty"` |
| type VersionElem struct { | ||
| // Version is the "x.x.x" version string. | ||
| Version string `json:"version"` | ||
| Date string `json:"date"` |
There was a problem hiding this comment.
We could docstring Date as well. From above, it's the published date.
| var blacklistedVersions = []version.Number{ | ||
| version.MustParse("4.0.0"), | ||
| version.MustParse("4.0.1"), | ||
| version.MustParse("4.0.2"), |
There was a problem hiding this comment.
tbh, this was just to showcase the blacklist. I guess the full list will go on until 4.1?
| } | ||
|
|
||
| client := j.GitHubClient | ||
| if client == nil { |
There was a problem hiding this comment.
i'm not a fan of this approach. i'd rather see the client passed in every time and fail if it is not, but ymmv
* feat(supported-versions): add SupportedVersions facade method (#1908) * feat(supported-versions): add SupportedVersions facade method * feat: remove errors.E (#1917) * chore: remove errors with code from tests * feat: remove errors.E * test: add looser checks to error message strings * chore: replace some errors.New usage with fmt.Errorf * feat: introduce errors.Codef function * refactor: revert loosening error message checks in tests * refactor: tweak db layer to return new errors when we know the code * refactor: tweak domain to use errors.Codef when we know the error code * chore: fix issues from rebase on v3 * chore: fix linter warning * test: tweaks to align with existing behaviour * chore: remove .* suffix introduced on some error checks * chore: simplify nesting of fmt.Sprintf inside fmt.Error * test: fix failing integration tests * test: add extra tests to errors package * test: move destroy-controller test to bootstrap test (#1924) * test: move destroy-controller test to bootstrap test * refactor: return error when watched job fails * test: update bootstrap test to use 3.6.19 * fix(trivy): update trivy action to latest (#1934)
* feat: add DB controller profile CRUD (#1904) * feat: add DB controller profile CRUD * chore: update test to expect Juju v3.6.19 * chore: add limits on DB fields * test: tweak upgrade test * feat: add API and domain methods for controller profiles (#1906) * feat: add API and domain methods for controller profiles * feat: add juju-version as a required field * chore: remove unnecessary errors.E wrapping * feat: expand bootstrap options for jaas bootstrap (#1913) * feat(supported-versions): add SupportedVersions facade method (#1908) * feat(supported-versions): add SupportedVersions facade method * refactor: adjust bootstrap cloud shape (#1921) * refactor: adjust bootstrap cloud shape * chore: remove unnecessary comments * feat: remove errors.E (#1917) * chore: remove errors with code from tests * feat: remove errors.E * test: add looser checks to error message strings * chore: replace some errors.New usage with fmt.Errorf * feat: introduce errors.Codef function * refactor: revert loosening error message checks in tests * refactor: tweak db layer to return new errors when we know the code * refactor: tweak domain to use errors.Codef when we know the error code * chore: fix issues from rebase on v3 * chore: fix linter warning * test: tweaks to align with existing behaviour * chore: remove .* suffix introduced on some error checks * chore: simplify nesting of fmt.Sprintf inside fmt.Error * test: fix failing integration tests * test: add extra tests to errors package * test: move destroy-controller test to bootstrap test (#1924) * test: move destroy-controller test to bootstrap test * refactor: return error when watched job fails * test: update bootstrap test to use 3.6.19 * feat: add additional flags to jaas bootstrap command (#1923) * feat: add additional flags to jaas bootstrap command * test: add test to verify space separated constraints * chore: add missing full stops * fix(trivy): update trivy action to latest (#1934) * docs: minor addition to manage controller doc (#1939) * feat(gh-caching): add caching to fetch github releases (#1938) * feat(gh-caching): add caching to fetch github releases * fix(deep-copy): deep copy before returning the cached slice * chore: remove errors.E usages * fix(vault-dockerfile): fix vault image (#1946) * fix(vault-dockerfile): remove install of bash/jq I think it was a left-over to do some initialization, but it's not used anymore and it was returning the error: [2/4] RUN apk add --no-cache bash jq: * fix(vault-dockerfile): pin vault image to 1 --------- Co-authored-by: Kian Parvin <46668016+kian99@users.noreply.github.com> Co-authored-by: Kian Parvin <kian.parvin@canonical.com>
* feat: add DB controller profile CRUD (#1904) * feat: add DB controller profile CRUD * chore: update test to expect Juju v3.6.19 * chore: add limits on DB fields * test: tweak upgrade test * feat: add API and domain methods for controller profiles (#1906) * feat: add API and domain methods for controller profiles * feat: add juju-version as a required field * chore: remove unnecessary errors.E wrapping * feat: expand bootstrap options for jaas bootstrap (#1913) * feat(supported-versions): add SupportedVersions facade method (#1908) * feat(supported-versions): add SupportedVersions facade method * refactor: adjust bootstrap cloud shape (#1921) * refactor: adjust bootstrap cloud shape * chore: remove unnecessary comments * feat: remove errors.E (#1917) * chore: remove errors with code from tests * feat: remove errors.E * test: add looser checks to error message strings * chore: replace some errors.New usage with fmt.Errorf * feat: introduce errors.Codef function * refactor: revert loosening error message checks in tests * refactor: tweak db layer to return new errors when we know the code * refactor: tweak domain to use errors.Codef when we know the error code * chore: fix issues from rebase on v3 * chore: fix linter warning * test: tweaks to align with existing behaviour * chore: remove .* suffix introduced on some error checks * chore: simplify nesting of fmt.Sprintf inside fmt.Error * test: fix failing integration tests * test: add extra tests to errors package * test: move destroy-controller test to bootstrap test (#1924) * test: move destroy-controller test to bootstrap test * refactor: return error when watched job fails * test: update bootstrap test to use 3.6.19 * feat: add additional flags to jaas bootstrap command (#1923) * feat: add additional flags to jaas bootstrap command * test: add test to verify space separated constraints * chore: add missing full stops * fix(trivy): update trivy action to latest (#1934) * docs: minor addition to manage controller doc (#1939) * feat(gh-caching): add caching to fetch github releases (#1938) * feat(gh-caching): add caching to fetch github releases * fix(deep-copy): deep copy before returning the cached slice * chore: remove errors.E usages * fix(vault-dockerfile): fix vault image (#1946) * fix(vault-dockerfile): remove install of bash/jq I think it was a left-over to do some initialization, but it's not used anymore and it was returning the error: [2/4] RUN apk add --no-cache bash jq: * fix(vault-dockerfile): pin vault image to 1 * chore: ran go fix ./... (#1947) * docs: add llms4 * docs: fix linkcheck issue * chore: update to go 1.26 * chore: ran go fix ./... and removed now unused Ptr function * ci: update golangci-lint version to v2.11.4 * chore: remove unused functions * refactor: tweaks for linter security warnings and ignores * chore: update dockerfile to use latest air and delve versions * fix: switch to external admin user (#1950) Change the user set as subject in the JWT when dialling to a Juju controller as self/admin. Previously it was `admin`, now it is `jaas-<jimm-controller-uuid>@external`. This ensures always we always dial as a Juju external user when using a JWT. * fix: delete dangling juju offer on fetch (#1951) When `GetApplicationOffer` is called, if the Juju controller reports there is no such offer, delete it from JIMM's database. This is dealing with a situation where a previous call to `DestroyOffer` had managed to call into the Juju controller, but the response timed out or similar. The endpoint then didn't end up deleting the offer from JIMM's database. * docs: update landing pages * docs: remove relic sections * docs: fix ci issue * feat: add cli commands for controller profiles (#1959) * feat: add cli commands for controller profiles * docs: update public godoc * docs: update purpose docs to remove "in jimm" * feat: add tabular formatting for list * docs: cli docs for controller profiles * chore: add links for new commands * fix: update go version (#1968) Update rock go version Update local dockerfile go version * ci: add a docker cache because sometimes pulling from docker is super slow (#1967) * ci: add a docker cache because sometimes pulling from docker is super slow * fix(remove): remove read-only * refactor(different-approach): use bare action/cache * feat(cache): consolidate places where cache is used and populated cache should be filled only in the cache.yaml workflow, in all the other instances, it's only used. * chore(merge): fix juju 3 to juju 4 transition for controller profiles --------- Co-authored-by: Kian Parvin <46668016+kian99@users.noreply.github.com> Co-authored-by: Kian Parvin <kian.parvin@canonical.com> Co-authored-by: Teodora Mihoc <teodora.mihoc@canonical.com> Co-authored-by: Ales Stimec <ales.stimec@canonical.com> Co-authored-by: Luci Brănescu <luci.branescu@canonical.com> Co-authored-by: Alex <42068202+ale8k@users.noreply.github.com>
* feat: add DB controller profile CRUD (#1904) * feat: add DB controller profile CRUD * chore: update test to expect Juju v3.6.19 * chore: add limits on DB fields * test: tweak upgrade test * feat: add API and domain methods for controller profiles (#1906) * feat: add API and domain methods for controller profiles * feat: add juju-version as a required field * chore: remove unnecessary errors.E wrapping * feat: expand bootstrap options for jaas bootstrap (#1913) * feat(supported-versions): add SupportedVersions facade method (#1908) * feat(supported-versions): add SupportedVersions facade method * refactor: adjust bootstrap cloud shape (#1921) * refactor: adjust bootstrap cloud shape * chore: remove unnecessary comments * feat: remove errors.E (#1917) * chore: remove errors with code from tests * feat: remove errors.E * test: add looser checks to error message strings * chore: replace some errors.New usage with fmt.Errorf * feat: introduce errors.Codef function * refactor: revert loosening error message checks in tests * refactor: tweak db layer to return new errors when we know the code * refactor: tweak domain to use errors.Codef when we know the error code * chore: fix issues from rebase on v3 * chore: fix linter warning * test: tweaks to align with existing behaviour * chore: remove .* suffix introduced on some error checks * chore: simplify nesting of fmt.Sprintf inside fmt.Error * test: fix failing integration tests * test: add extra tests to errors package * test: move destroy-controller test to bootstrap test (#1924) * test: move destroy-controller test to bootstrap test * refactor: return error when watched job fails * test: update bootstrap test to use 3.6.19 * feat: add additional flags to jaas bootstrap command (#1923) * feat: add additional flags to jaas bootstrap command * test: add test to verify space separated constraints * chore: add missing full stops * fix(trivy): update trivy action to latest (#1934) * docs: minor addition to manage controller doc (#1939) * feat(gh-caching): add caching to fetch github releases (#1938) * feat(gh-caching): add caching to fetch github releases * fix(deep-copy): deep copy before returning the cached slice * chore: remove errors.E usages * fix(vault-dockerfile): fix vault image (#1946) * fix(vault-dockerfile): remove install of bash/jq I think it was a left-over to do some initialization, but it's not used anymore and it was returning the error: [2/4] RUN apk add --no-cache bash jq: * fix(vault-dockerfile): pin vault image to 1 * chore: ran go fix ./... (#1947) * docs: add llms4 * docs: fix linkcheck issue * chore: update to go 1.26 * chore: ran go fix ./... and removed now unused Ptr function * ci: update golangci-lint version to v2.11.4 * chore: remove unused functions * refactor: tweaks for linter security warnings and ignores * chore: update dockerfile to use latest air and delve versions * fix: switch to external admin user (#1950) Change the user set as subject in the JWT when dialling to a Juju controller as self/admin. Previously it was `admin`, now it is `jaas-<jimm-controller-uuid>@external`. This ensures always we always dial as a Juju external user when using a JWT. * fix: delete dangling juju offer on fetch (#1951) When `GetApplicationOffer` is called, if the Juju controller reports there is no such offer, delete it from JIMM's database. This is dealing with a situation where a previous call to `DestroyOffer` had managed to call into the Juju controller, but the response timed out or similar. The endpoint then didn't end up deleting the offer from JIMM's database. * docs: update landing pages * docs: remove relic sections * docs: fix ci issue * feat: add cli commands for controller profiles (#1959) * feat: add cli commands for controller profiles * docs: update public godoc * docs: update purpose docs to remove "in jimm" * feat: add tabular formatting for list * docs: cli docs for controller profiles * chore: add links for new commands * fix: update go version (#1968) Update rock go version Update local dockerfile go version * ci: add a docker cache because sometimes pulling from docker is super slow (#1967) * ci: add a docker cache because sometimes pulling from docker is super slow * fix(remove): remove read-only * refactor(different-approach): use bare action/cache * feat(cache): consolidate places where cache is used and populated cache should be filled only in the cache.yaml workflow, in all the other instances, it's only used. --------- Co-authored-by: SimoneDutto <simone.dutto@canonical.com> Co-authored-by: Teodora Mihoc <teodora.mihoc@canonical.com> Co-authored-by: Ales Stimec <ales.stimec@canonical.com> Co-authored-by: Luci Brănescu <luci.branescu@canonical.com> Co-authored-by: Alex <42068202+ale8k@users.noreply.github.com>
Description
SupportedVersionsreturns the list of Juju versions that JAAS/JIMM considers supported.It calls the GitHub API to fetch the releases, it does filtering, and returns the list.
API shape
I like the idea of the optional contextualVersion (stealing the concept from
ContextualTuplesin Openfga, so if they send it to us we can potentially build some logic around suggested upgrade, etc.Previous implementation
Instead of querying GitHub at runtime from the facade, we now generate a JSON file at build time (via
make generate) and embed it into thejimmbinary (and also keep the generated JSON in-repo for visibility / traceability).The why
What i like:
release.jsonshipped with 3.3.19 and confirm that version was never tested against that version of JIMM.What i don't like too much:
make generateand commit the updated JSON.This is acceptable and even useful because it makes Juju version bumps explicit, and it aligns with how we run e2e:
stablesnap track update.Let me know if you like it or not.