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

Improvements and bug fixes #47

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gdbranco
Copy link
Contributor

What

  • feat: allows to check for resource review permission against OCM AMS
  • feat: includes migration test to validate new files are included as functions
  • feat: allow a more dynamic kind and path based on resource type
  • fix: allows to search for exposed nested resources and doesn't expose internal sql columns

Tests

Unit

OCM_ENV=testing gotestsum --format short-verbose -- -p 1 -v  \
	./pkg/... \
	./cmd/...
EMPTY pkg/api
EMPTY pkg/api/openapi
PASS pkg/api/presenters.TestAccessProtection (0.00s)
PASS pkg/api/presenters (cached)
EMPTY pkg/auth
EMPTY pkg/client/ocm
PASS pkg/config.TestConfigReadStringFile (0.00s)
PASS pkg/config.TestConfigReadIntFile (0.00s)
PASS pkg/config.TestConfigReadBoolFile (0.00s)
PASS pkg/config.TestConfigReadQuotedFile (0.00s)
PASS pkg/config (cached)
PASS pkg/controllers.TestControllerFramework (0.00s)
PASS pkg/controllers (cached)
PASS pkg/dao.TestAccessProtection (0.00s)
PASS pkg/dao (cached)
EMPTY pkg/dao/mocks
EMPTY pkg/db
EMPTY pkg/db/db_context
EMPTY pkg/db/db_session
PASS pkg/db/migrations.TestMigration (0.00s)
PASS pkg/db/migrations (cached)
EMPTY pkg/db/mocks
EMPTY pkg/db/transaction
PASS pkg/errors.TestErrorFormatting (0.00s)
PASS pkg/errors.TestErrorFind (0.00s)
PASS pkg/errors (cached)
EMPTY pkg/handlers (cached)
EMPTY pkg/logger
PASS pkg/services.TestDinosaurFindBySpecies (0.00s)
PASS pkg/services.TestServices (3.00s)
PASS pkg/services
PASS pkg/util.TestAccessProtection (0.00s)
PASS pkg/util (cached)
EMPTY pkg/util/test
EMPTY cmd/trex
EMPTY cmd/trex/clone
PASS cmd/trex/environments.TestLoadServices (1.12s)
PASS cmd/trex/environments
EMPTY cmd/trex/migrate
EMPTY cmd/trex/servecmd
EMPTY cmd/trex/server
EMPTY cmd/trex/server/logging

DONE 14 tests in 6.687s

Integration

OCM_ENV=testing gotestsum --format short-verbose -- -p 1 -ldflags -s -v -timeout 1h  \
		./test/integration
I0613 13:52:15.317050  521034 framework.go:72] Initializing testing environment
I0613 13:52:16.481160  521034 framework.go:151] Using Mock OCM Authz Client
I0613 13:52:16.481191  521034 framework.go:176] Disabling Sentry error reporting
I0613 13:52:16.498173  521034 api_server.go:145] Serving without TLS at localhost:8000
I0613 13:52:16.499025  521034 healthcheck_server.go:56] Serving HealthCheck without TLS at localhost:8083
PASS test/integration.TestControllerRacing (2.69s)
PASS test/integration.TestDinosaurGet (1.25s)
PASS test/integration.TestDinosaurPost (1.19s)
PASS test/integration.TestDinosaurPatch (1.25s)
PASS test/integration.TestDinosaurPaging (1.40s)
PASS test/integration.TestDinosaurListSearch (1.51s)
PASS test/integration.TestUpdateDinosaurWithRacingRequests (1.32s)
PASS test/integration.TestUpdateDinosaurWithRacingRequests_WithoutLock (1.27s)
I0613 13:52:28.393263  521034 api_server.go:151] Web server terminated
PASS test/integration

DONE 8 tests in 14.196s

@gdbranco gdbranco force-pushed the feat/improvements-and-bug-fixes branch from c90765f to d74fd81 Compare June 13, 2024 16:55
)

var (
dinosaurTableName = util.ToSnakeCase(api.DinosaurTypeName) + "s"
dinosaurColumns = []string{
Copy link
Contributor

@tiwillia tiwillia Jun 14, 2024

Choose a reason for hiding this comment

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

This means to support searching a new column, it must be added to this list?

I see clusters-service implementing this requirement in this way, but AMS/OSDFM seem to handle this differently.

I'm not sure which is better tbh, the AMS implementation is explicit while this implementation is simpler. @markturansky can you provide your input here?

Copy link
Contributor Author

@gdbranco gdbranco Jun 17, 2024

Choose a reason for hiding this comment

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

Yeah, I'd like to eventually be able to move away from this to have something that uses reflect or similar to have it be more dynamic

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd prefer to not explicitly set casing, as that negates the casing config allowed by gorm (that also does things like table name prefix, etc). can we do this through gorm somehow,?

@gdbranco gdbranco requested a review from markturansky June 17, 2024 11:15
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