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

Validate api client imports #16634

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Conversation

SimonRichardson
Copy link
Member

@SimonRichardson SimonRichardson commented Nov 28, 2023

To ensure that we don't accidentally bring in an import for the api client packages, I've added a static analysis to validate that.

In PR[1] it was noticed that we brought in the whole of the aws library. That wasn't intentional, so we need to prevent ourselves from doing it again. This should hopefully go some way to do that.

  1. Removes aws s3 dependency on api client #16633

Checklist

  • Code style: imports ordered, good names, simple structure, etc
  • Comments saying why design decisions were made

QA steps

$ make static-analysis

If you apply this without rebasing, you'll see the following error:

➜ make static-analysis
dqlite already installed

==> Checking for dependencies
==> Using Juju located at /home/simon/go/bin/juju
==> TEST BEGIN: test_static_analysis (tmp.Tnh)
===> [ ✔ ] Success: copyright (1s)
===> [ ✔ ] Success: check doc go (10s)
===> [ ✔ ] Success: api client (9s)
===> [ x ] Fail: go (1s)
==> Cleaning up
====> Completed cleaning up pids
====> Completed cleaning up jujus

==> TESTS DONE: test_static_analysis
==> RUN OUTPUT: test_static_analysis
    | Checking ./api/client/action
    | Checking ./api/client/annotations
    | Checking ./api/client/application
    | Checking ./api/client/applicationoffers
    | Checking ./api/client/backups
    | Checking ./api/client/block
    | Checking ./api/client/bundle
    | Checking ./api/client/charms
    | Import not allowed: github.com/aws/aws-sdk-go-v2
    | Consult the list of allowed imports.
    | Error: API Client import failure in ./api/client/charms
    | Checking ./api/client/client
    | Checking ./api/client/cloud
    | Checking ./api/client/credentialmanager
    | Checking ./api/client/highavailability
    | Checking ./api/client/imagemetadatamanager
    | Checking ./api/client/keymanager
    | Checking ./api/client/machinemanager
    | Checking ./api/client/metricsdebug
    | Checking ./api/client/modelconfig
    | Checking ./api/client/modelgeneration
    | Checking ./api/client/modelmanager
    | Checking ./api/client/modelupgrader
    | Checking ./api/client/payloads
    | Checking ./api/client/resources
    | Checking ./api/client/secretbackends
    | Checking ./api/client/secrets
    | Checking ./api/client/spaces
    | Checking ./api/client/sshclient
    | Checking ./api/client/storage
    | Checking ./api/client/subnets
    | Checking ./api/client/usermanager

Links

Jira card: JUJU-5085

Copy link
Member

@nvinuesa nvinuesa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this improvement! only one small naming comment

tests/suites/static_analysis/lint_go.sh Outdated Show resolved Hide resolved
To ensure that we don't accidently bring in an import for the api
client packages, I've added a static analysis to validate that.

In PR[1] it was noticed that we brought in the whole of the aws
library. That wasn't intentional, so we need to prevent ourselves
from doing it again. This should hopefully go someway to do that.

 1. juju#16633
Checking the base package also, as the client will also use that
package.
We require docker for some reason in the api packages. This is clearly
wrong, we should fix this with some urgency.
@SimonRichardson
Copy link
Member Author

/merge

@jujubot jujubot merged commit 55cabf0 into juju:3.3 Nov 29, 2023
16 of 20 checks passed
@SimonRichardson SimonRichardson deleted the check-client-imports branch November 29, 2023 12:24
@wallyworld wallyworld mentioned this pull request Jan 2, 2024
jujubot added a commit that referenced this pull request Jan 2, 2024
#16720

Merge 3.3

Conflicts were imports and go mod.

#16637 [from hpidcock/improve-indexes](07162f1)
#16632 [from SimonRichardson/fix-spaces-matcher](e88b7d2)
#16634 [from SimonRichardson/check-client-imports](55cabf0)
#16646 [from nvinuesa/backport_nic_name_assertion](b843d60)
#16651 [from nvinuesa/juju-5092](c1e063c)
#16652 [from hpidcock/fix-sidecar-teardown-app-dead](070f3e8)
#16653 [from ycliuhw/simplify-vault4CI](e185779)
#16604 [from ycliuhw/secret-grant-info](ffa2ac9)
#16513 [from hmlanigan/implement-todo](0302c4a)
#16664 [from jack-w-shaw/JUJU-5053_print_action_fai…](1b140b8)
#16667 [from jack-w-shaw/JUJU-5053_update_action_he…](0cd207d)
#16680 [from hpidcock/fix-machine-lock-log-dir](0552528)
#16704 [from wallyworld/appoffertag-uuid](627357c)
#16706 [from wallyworld/update-names-v5](75c2f63)
#16719 [from wallyworld/gomod-updates](246a6e9)
#16563 [from wallyworld/remote-relations-doc](34c1fa9)
#16568 [from cderici/add-instancemutater-doc](67a0b72)

```
# Conflicts:
# api/agent/uniter/relation_test.go
# api/agent/uniter/relationunit_test.go
# api/client/payloads/client_test.go
# api/client/payloads/helpers_test.go
# api/watcher/watcher_test.go
# apiserver/common/crossmodel/interface.go
# apiserver/common/firewall/firewall_test.go
# apiserver/facades/agent/caasapplication/state.go
# apiserver/facades/agent/caasoperator/state.go
# apiserver/facades/agent/provisioner/interface.go
# apiserver/facades/client/application/mocks/lxdprofile_mock.go
# apiserver/facades/client/applicationoffers/base_test.go
# apiserver/facades/client/bundle/bundle_test.go
# apiserver/facades/client/bundle/mock_test.go
# apiserver/facades/client/bundle/state.go
# apiserver/facades/client/client/api_test.go
# apiserver/facades/client/controller/backend.go
# apiserver/facades/client/modelgeneration/interface.go
# apiserver/facades/controller/caasapplicationprovisioner/state.go
# apiserver/facades/controller/caasfirewaller/firewaller_test.go
# apiserver/facades/controller/caasfirewaller/mock_test.go
# apiserver/facades/controller/charmrevisionupdater/interface_test.go
# apiserver/facades/controller/crossmodelrelations/crossmodelrelations_test.go
# cmd/juju/application/deployer/mocks/charm_mock.go
# cmd/juju/application/refresher/charm_mock_test.go
# cmd/juju/application/store/mocks/charm_mock.go
# cmd/juju/resource/formatter_test.go
# core/charm/charm_mock_test.go
# core/model/charm_mock_test.go
# go.mod
# go.sum
# migration/migration.go
# migration/migration_test.go
# resource/export_test.go
# state/migration_export_test.go
# state/migration_import_test.go
# state/migrations/remoteapplications.go
# testing/factory/factory.go
# worker/metrics/spool/metrics_test.go
```
@wallyworld wallyworld mentioned this pull request Jan 3, 2024
jujubot added a commit that referenced this pull request Jan 3, 2024
#16724

Merge 3.4

#16637 [from hpidcock/improve-indexes](07162f1)
#16632 [from SimonRichardson/fix-spaces-matcher](e88b7d2)
#16634 [from SimonRichardson/check-client-imports](55cabf0a82322f0b5807e5bfeb0f347cc9e9ecea)https://github.com/juju/juju/pull/16634 [from SimonRichardson/check-client-imports](55cabf0)
#16651 [from nvinuesa/juju-5092](c1e063c)
#16652 [from hpidcock/fix-sidecar-teardown-app-dead](070f3e8)
#16653 [from ycliuhw/simplify-vault4CI](e185779)
#16604 [from ycliuhw/secret-grant-info](ffa2ac9)
#16513 [from hmlanigan/implement-todo](0302c4a)
#16664 [from jack-w-shaw/JUJU-5053_print_action_fai…](1b140b8)
#16667 [from jack-w-shaw/JUJU-5053_update_action_he…](0cd207d)
#16428 [from benhoyt/pebble-notices](6d7ceae)
#16706 [from wallyworld/update-names-v5](75c2f63)
#16719 [from wallyworld/gomod-updates](246a6e9)
#16704 [from wallyworld/appoffertag-uuid](627357c)
#16701 [from jack-w-shaw/JUJU-4960_break_up_charm_api](b735335)

Conflicts were mainly imports, plus changes where state has been removed in 4.0

```
# Conflicts:
# agent/addons/addons.go
# agent/addons/engine_mock_test.go
# agent/agentbootstrap/bootstrap_test.go
# agent/engine/metrics.go
# api/agent/agent/machine_test.go
# api/agent/agent/unit_test.go
# api/agent/caasoperator/client.go
# api/agent/caasoperator/client_test.go
# api/agent/deployer/deployer.go
# api/agent/deployer/unit.go
# api/agent/diskmanager/diskmanager.go
# api/agent/hostkeyreporter/facade.go
# api/agent/instancemutater/mocks/caller_mock.go
# api/agent/machiner/machiner_test.go
# api/agent/provisioner/mocks/caller_mock.go
# api/agent/provisioner/provisioner_test.go
# api/agent/secretsdrain/mocks/facade_mock.go
# api/agent/uniter/podspec_test.go
# api/base/mocks/caller_mock.go
# api/base/mocks/clientfacade_mock.go
# api/client/charms/client.go
# api/client/charms/mocks/objectstore_mock.go
# api/client/modelupgrader/mocks/apibase_mock.go
# api/client/modelupgrader/mocks/context_mock.go
# api/common/secretbackends/mocks/facade_mock.go
# api/common/secretsdrain/mocks/facade_mock.go
# api/controller/caasoperatorprovisioner/client.go
# api/controller/caasoperatorprovisioner/client_test.go
# api/controller/caasunitprovisioner/client_test.go
# api/controller/controller/legacy_test.go
# api/controller/firewaller/machine_test.go
# api/controller/usersecretsdrain/mocks/facade_mock.go
# api/export_test.go
# api/state_test.go
# api/watcher/watcher_test.go
# apiserver/admin.go
# apiserver/apiserver_test.go
# apiserver/common/action.go
# apiserver/common/credentialcommon/mocks/credentialcommon_mock.go
# apiserver/common/credentialcommon/modelcredential.go
# apiserver/common/credentialcommon/modelcredential_test.go
# apiserver/common/crossmodel/crossmodel.go
# apiserver/common/crossmodel/interface.go
# apiserver/common/crossmodel/state.go
# apiserver/common/instanceidgetter.go
# apiserver/common/mocks/controllerconfig_mock.go
# apiserver/common/mocks/storage.go
# apiserver/common/mocks/tools_mock.go
# apiserver/common/modelmachineswatcher_test.go
# apiserver/common/secrets/drain.go
# apiserver/common/secrets/drain_test.go
# apiserver/common/secrets/mocks/commonsecrets_mock.go
# apiserver/common/secrets/mocks/provider_mock.go
# apiserver/common/secrets/state.go
# apiserver/export_test.go
# apiserver/facade/interface.go
# apiserver/facade/mocks/facade_mock.go
# apiserver/facades/agent/caasapplication/application_test.go
# apiserver/facades/agent/caasoperator/mock_test.go
# apiserver/facades/agent/caasoperator/operator.go
# apiserver/facades/agent/caasoperator/operator_test.go
# apiserver/facades/agent/caasoperator/state.go
# apiserver/facades/agent/credentialvalidator/backend.go
# apiserver/facades/agent/credentialvalidator/backend_test.go
# apiserver/facades/agent/credentialvalidator/state.go
# apiserver/facades/agent/diskmanager/diskmanager.go
# apiserver/facades/agent/hostkeyreporter/facade.go
# apiserver/facades/agent/hostkeyreporter/facade_test.go
# apiserver/facades/agent/instancemutater/instancemutater_test.go
# apiserver/facades/agent/instancemutater/lxdprofilewatcher.go
# apiserver/facades/agent/instancemutater/mocks/instancemutater_mock.go
# apiserver/facades/agent/keyupdater/authorisedkeys_test.go
# apiserver/facades/agent/logger/logger_test.go
# apiserver/facades/agent/machineactions/machineactions.go
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants