Arvados

developer-run-tests: #4903

23181-credentials-api @ 91b13ca08b70ad102cc15e50cf0c13942eb3a025

developer-run-tests: #4905

• ✅ All agreed upon points are implemented / addressed.
  • Added non-null validation of name, credential_class, external_id, secret, and expires_at to the model
  • Added migration so the db now requires the same fields
  • Up and down migrations were created, even though the down migration probably isn't strictly necessary. The up migration includes backfill that assigns any null values to empty strings. This is necessary on our dev cluster, which already has some null values, but customers will never notice.
  • created unit and integration tests
  • updated documentation
• n/a Anything not implemented (discovered or discussed during work) has a follow-up story.
• ✅ Code is tested and passing, both automated and manual, what manual testing was done is described
  • Manually tested by trying to create credentials with missing fields before and after applying changes
• ✅ The tested code incorporates recent main branch changes.
• n/a New or changed UX/UX and has gotten feedback from stakeholders.
• ✅ Documentation has been updated.
• ✅ Behaves appropriately at the intended scale (describe intended scale).
  • Scale is a single user creating/updating a single credential at a time
• ✅ Considered backwards and forwards compatibility issues between client and server.
  • Should be no change as the client enforces required fields when submitting and accounts for null fields when rendering.
• ✅ Follows our coding standards and GUI style guidelines.

This might be bikeshedding but I wish the resulting table definitions looked like other tables/columns ("name character varying NOT NULL" instead of named constraints). I think this would do it?

ALTER TABLE credentials
  ALTER COLUMN name {SET/DROP} NOT NULL
  ...
  ;

It finally occurred to me that the data migration is somewhat academic in that the previous release (3.1) didn't even have a credentials API, but I'm going to mention anyway: It looks like we're still accepting empty strings for new records, but the migration backfills NULL entries with 'Unnamed', 'unknown', etc. If empty strings are allowed, then I think it would be less weird to backfill with empty strings.

If the intent of the API is to require a non-empty name, we should also validate that, and mention it on the doc page. Since the name is unique per cluster, I think that is the intent.

We should add validates :scopes, array_of_strings: true -- looks like I missed doing that in #23152. Otherwise we'd accidentally accept stuff like ["foo", ["bar"]] and ["foo", null].

Please remove the "\restrict" and "\unrestrict" and "Dumped from/by ..." lines from source:services/api/db/structure.sql. See #23195.

Tom Clegg wrote in #note-8:

If the intent of the API is to require a non-empty name, we should also validate that, and mention it on the doc page. Since the name is unique per cluster, I think that is the intent.

This is the intent, for all the string fields. Hence "confirm that all required fields are nonblank" in #note-2. (To be sure, that thought should've been backfilled to the ticket description, it's not sufficiently clear on this point. Mea culpa.)

23181-credentials-api @ a1a4e61c1cde20749cc9b743c282aedecf3f9ecb

developer-run-tests: #4915

This might be bikeshedding but I wish the resulting table definitions looked...

Done

If empty strings are allowed, then I think it would be less weird to backfill with empty strings...

Done

If the intent of the API is to require a non-empty name, we should also validate that...

Done, including strings with tabs or spaces, with the appropriate tests. This validation is only done in the model and not at the db level. To do so at the db level would be nice, but that would conflict with the migration backfill to empty strings. If we are super concerned about db-level validation, we have to backfill to 'Unnamed', 'unknown', etc.

We should add validates :scopes, array_of_strings: true...

Done, with the appropriate tests.

Please remove the "\restrict" and "\unrestrict" and "Dumped from/by ..."

Done

test "credential scopes must be an array of strings or nil" in source:services/api/test/integration/credentials_test.rb seems quite repetitive -- maybe the credential attrs other than scopes could be stated once and reused, to make it easier to spot the differences between the cases?

Related: I don't mind the tests as written, but as a general note, Ruby does make it easy to write parameterized tests as separate named test cases, which can be helpful because the test run output shows the full list of which of the N variants failed, instead of stopping at the first failure. e.g., in source:services/api/test/unit/container_test.rb

  [
    # ...
    [Container::Complete, {state: Container::Cancelled}],
    [Container::Complete, {priority: 123456789}],
    [Container::Complete, {runtime_status: {'error' => 'oops'}}],
    [Container::Complete, {cwd: '/'}],
    [Container::Cancelled, {cwd: '/'}],
  ].each do |start_state, updates|
    test "Container update #{updates.inspect} when #{start_state} forbidden for non-admin" do

Rest LGTM, thanks.

23181-credentials-api @ 92e83e049a8104b72b8310128017304b29003721

developer-run-tests: #4919

Succinctified test "credential scopes must be an array of strings or nil"

LGTM, thanks!

23181-credentials-api @ 24ee16ee2600d2668463861078c988f9539704c2

developer-run-tests: #4920

Merged main in to incorporate flaky test fixes

Lucas Di Pentima wrote in #note-17:

I'm seeing the following error when attempting to apply the last added migration to tordo:

[...]

Given that the credentials tables is new, and no production cluster has it, could we just delete every record in the credentials table instead of updating them, to avoid conflicts?

@Lisa Knox deleted the offending records, and I was able to run db:migrate successfully.
Re-running the deployment pipeline: deploy-to-tordo: #1928