[STCLI-148] optional-dependency syntax in MDs is incorrect Created: 23/Apr/20  Updated: 24/Apr/20  Resolved: 23/Apr/20

Status: Closed
Project: stripes-cli
Components: None
Affects versions: None
Fix versions: 1.15.1

Type: Bug Priority: P2
Reporter: Zak Burke Assignee: Mike Taylor
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original estimate: Not Specified

Issue links:
Blocks
blocks FOLIO-2569 Failing UI regression tests on platfo... Closed
Relates
relates to OKAPI-509 Optional dependencies. "optional" pro... Closed
relates to FOLIO-2572 Update jenkins-slave-all Jenkins dock... Closed
Sprint:

 Description   

CI cannot complete`master` branch builds of ui-tenant-settings because module descriptor publishing fails. It appears not to like the new `optionalOkapiInterfaces` that are now present in the `package.json`:

    "optionalOkapiInterfaces": {
      "course-reserves-storage": "0.2",
      "reserves-storage": "0.1"
    },

as the build log reports:

HttpMethod: POST
URL: http://folio-registry.aws.indexdata.com/_/proxy/modules
Content-Type: application/json; charset=UTF-8
Accept: application/json
Sending request to url: http://folio-registry.aws.indexdata.com/_/proxy/modules
Response Code: HTTP/1.1 400 Bad Request
Response: 
Failed to decode:Unrecognized field "optional" (class org.folio.okapi.bean.InterfaceDescriptor), not marked as ignorable (5 known properties: "version", "handlers", "id", "scope", "interfaceType"])


 Comments   
Comment by Zak Burke [ 23/Apr/20 ]

Mike Taylor, any thoughts? I know you did some related work in stripes-cli for OKAPI-509 Closed , though this seems to be a problem with Okapi not liking the optional interfaces, rather than stripes-cli not knowing how to handle them.

Comment by Mike Taylor [ 23/Apr/20 ]

Agreed, this seems to be a problem in Okapi.

Comment by Marc Johnson [ 23/Apr/20 ]

The error reported in the issue suggests that Okapi is refusing the descriptor because it does not recognise a property.

That either means that:

  • Okapi does not support optional dependencies yet (or at least the version running the registry)
  • There is a bug in Okapi's support for optional dependencies
  • The descriptor is defining optional dependencies incorrectly

John Malconian Ian Hardy What version of Okapi is the registry running? Is it 2.37.0 or later?

Comment by Marc Johnson [ 23/Apr/20 ]

Zak Burke Mike Taylor

Please can an example module descriptor be included.

The error message suggests that the optional property is being included at the same level as version, handlers, id, scope, interfaceType which are properties of the interface descriptor schema (used for both provided interfaces and dependencies). Is it being included as a boolean?

The Module Descriptor schema suggests that this is actually modelled by a separate array object named optional at a higher level.

This automated test seems to confirm that.

That suggests to me that the client, the UI module build, is generating an invalid module descriptor and Okapi is correct in refusing it.

Comment by Zak Burke [ 23/Apr/20 ]

Marc Johnson, the MD from linked build failure is copied below. Mike Taylor, does Marc's assessment make sense?

{
  "id": "folio_tenant-settings-3.0.100096",
  "name": "Tenant settings",
  "permissionSets": [
    {
      "permissionName": "module.tenant-settings.enabled",
      "displayName": "UI: Tenant-settings module is enabled"
    },
    {
      "permissionName": "settings.tenant-settings.enabled",
      "displayName": "Settings (tenant): display list of settings pages",
      "subPermissions": [
        "settings.enabled"
      ]
    },
    {
      "permissionName": "ui-tenant-settings.settings.addresses",
      "displayName": "Settings (tenant): Can manage tenant addresses",
      "subPermissions": [
        "configuration.all",
        "users.collection.get",
        "settings.tenant-settings.enabled"
      ],
      "visible": true
    },
    {
      "permissionName": "ui-tenant-settings.settings.key-bindings",
      "displayName": "Settings (tenant): Can maintain key bindings",
      "subPermissions": [
        "configuration.all",
        "settings.tenant-settings.enabled"
      ],
      "visible": true
    },
    {
      "permissionName": "ui-tenant-settings.settings.locale",
      "displayName": "Settings (tenant): Can edit language, localization, and currency",
      "subPermissions": [
        "configuration.all",
        "settings.tenant-settings.enabled"
      ],
      "visible": true
    },
    {
      "permissionName": "ui-tenant-settings.settings.plugins",
      "displayName": "Settings (tenant): Can maintain preferred plugins",
      "subPermissions": [
        "configuration.all",
        "settings.tenant-settings.enabled"
      ],
      "visible": true
    },
    {
      "permissionName": "ui-tenant-settings.settings.sso",
      "displayName": "Settings (tenant): Can maintain SSO settings",
      "subPermissions": [
        "configuration.all",
        "settings.tenant-settings.enabled"
      ],
      "visible": true
    },
    {
      "permissionName": "ui-tenant-settings.settings.location",
      "displayName": "Settings (tenant): Can create, edit and remove locations",
      "subPermissions": [
        "configuration.all",
        "settings.tenant-settings.enabled",
        "users.item.get",
        "users.collection.get",
        "inventory-storage.location-units.institutions.collection.get",
        "inventory-storage.location-units.institutions.item.get",
        "inventory-storage.location-units.institutions.item.post",
        "inventory-storage.location-units.institutions.item.put",
        "inventory-storage.location-units.institutions.item.delete",
        "inventory-storage.location-units.campuses.collection.get",
        "inventory-storage.location-units.campuses.item.get",
        "inventory-storage.location-units.campuses.item.post",
        "inventory-storage.location-units.campuses.item.put",
        "inventory-storage.location-units.campuses.item.delete",
        "inventory-storage.location-units.libraries.collection.get",
        "inventory-storage.location-units.libraries.item.get",
        "inventory-storage.location-units.libraries.item.post",
        "inventory-storage.location-units.libraries.item.put",
        "inventory-storage.location-units.libraries.item.delete",
        "inventory-storage.locations.collection.get",
        "inventory-storage.locations.item.get",
        "inventory-storage.locations.item.post",
        "inventory-storage.locations.item.put",
        "inventory-storage.locations.item.delete"
      ],
      "visible": true
    },
    {
      "permissionName": "ui-tenant-settings.settings.servicepoints",
      "displayName": "Settings (tenant):  Can create, edit and remove service points",
      "subPermissions": [
        "configuration.all",
        "settings.tenant-settings.enabled",
        "users.item.get",
        "users.collection.get",
        "inventory-storage.locations.collection.get",
        "inventory-storage.location-units.institutions.collection.get",
        "inventory-storage.location-units.campuses.collection.get",
        "inventory-storage.location-units.libraries.collection.get",
        "inventory-storage.service-points.collection.get",
        "inventory-storage.service-points.item.get",
        "inventory-storage.service-points.item.post",
        "inventory-storage.service-points.item.put",
        "inventory-storage.service-points.item.delete"
      ],
      "visible": true
    }
  ],
  "requires": [
    {
      "id": "configuration",
      "version": "2.0"
    },
    {
      "id": "location-units",
      "version": "2.0"
    },
    {
      "id": "locations",
      "version": "3.0"
    },
    {
      "id": "login-saml",
      "version": "1.0"
    },
    {
      "id": "service-points",
      "version": "3.0"
    },
    {
      "id": "users",
      "version": "15.0"
    },
    {
      "id": "course-reserves-storage",
      "version": "0.2",
      "optional": true
    },
    {
      "id": "reserves-storage",
      "version": "0.1",
      "optional": true
    }
  ]
}
Comment by Mike Taylor [ 23/Apr/20 ]

Note that you can easily see generated descriptors for yourself:
```
$ yarn global add @folio/stripes-cli
$ cd ~/git/folio/stripes/ui-tenant-settings
$ stripes mod descriptor --full --strict
```
(You need v15.x of the Stripes CLI.)

Comment by Mike Taylor [ 23/Apr/20 ]

But, yes, Marc's analysis looks right. It seems that what the Stripes CLI generates is not the format that Okapi expects. I can fix that.

Comment by Marc Johnson [ 23/Apr/20 ]

Should this issue be moved to the project for stripes-cli?

Comment by Mike Taylor [ 23/Apr/20 ]

Too late, I fixed it: https://github.com/folio-org/stripes-cli/pull/218

Comment by John Malconian [ 23/Apr/20 ]

the FOLIO registry is running okapi 2.38.0.

Comment by Marc Johnson [ 23/Apr/20 ]

John Malconian

I think this can be closed as cannot reproduce, the issue was with the stripes-cli not Okapi, and Mike has already worked on a change for that separately

Comment by Mike Taylor [ 23/Apr/20 ]

Don't you think that verifying that my fix works is part of the present issue?

Comment by Marc Johnson [ 23/Apr/20 ]

Mike Taylor

Don't you think that verifying that my fix works is part of the present issue?

If you want to use this issue to manage your fix, then I don't mind you re-opening it. If you do, please could it be moved to the appropriate project and updated the reflect that the cause.

Comment by Mike Taylor [ 23/Apr/20 ]

Well, I'm not going to get involved in devops work, which is what this is now.

I guess if you guys want to close this and open another, I won't stand in your way.

Comment by Marc Johnson [ 23/Apr/20 ]

Mike Taylor

Well, I'm not going to get involved in devops work, which is what this is now.

I don't follow. How is this DevOps work?

Comment by Mike Taylor [ 23/Apr/20 ]

First line of the description: "CI cannot complete`master` branch builds of ui-tenant-settings". That's devops, isn't it? The obvious way to check that the new format is correct would be to re-do the CI `master` branch build of ui-tenant-settings.

Comment by Marc Johnson [ 23/Apr/20 ]

Mike Taylor

First line of the description: "CI cannot complete`master` branch builds of ui-tenant-settings". That's devops, isn't it?

My understanding is that was a description of the symptoms of the issue. Zak Burke may have had an inkling that the cause was the CI process or Okapi (hence the issue being posted to the #devops and #development channels for help).

Given that we now know that the cause is that ui-tenant-settings (using stripes-cli) was generating an invalid descriptor (and hence Okapi and CI were doing the right thing), I would say that either this issue be closed as not relevant to the CI process or Okapi, or that it be moved to the project where the issue and the fix can be done.

The obvious way to check that the new format is correct would be to re-do the CI `master` branch build of ui-tenant-settings

Personally, I think the best way to check that this is resolved would be to validate that the generated descriptor is valid. I think that can be done without any involvement of the CI process.

I think it's best I stop, I likely shouldn't have asked the previous question. I don't think I'm adding any value to you or Zak Burke's day by prolonging this. Apologies

Comment by Mike Taylor [ 23/Apr/20 ]

I think the best way to check that this is resolved would be to validate that the generated descriptor is valid. I think that can be done without any involvement of the CI process.

Yes; but not without me building and running an Okapi (or someone else doing it).

Anyway, let's wait for Zak Burke to turn up here and propose a way forward.

Comment by Marc Johnson [ 23/Apr/20 ]

Apologies to all, I should've left this issue open and stepped away once I'd helped diagnose the issue

Comment by Mike Taylor [ 23/Apr/20 ]

I reject your apology! It is by no means necessary. Thank you for contributing.

Comment by Zak Burke [ 23/Apr/20 ]

The original description is the behavior I saw in Jenkins (hence filing this as a FOLIO ticket and notifying #dev-ops). But then I realized the problem was not a process problem but that okapi was rejecting the generated MD (hence asking if this should be moved to OKAPI and notifying #development). But then we realized this was a problem with stripes-cli incorrectly generating MDs, which were generated during Jenkins builds, and then passed to Okapi, hence this step:

I'm going to edit the description, re-open it, and move it to stripes-cli.

Whether we should have opened new tickets at each stage of the diagnosis or moved this one along is a question we deal with all the time, e.g. "some UI app shows some glitch which is a result of data it received from the backend and formatted with a component from stripes-components, so which should the ticket be filed against?" There is more than one possible answer, no single right answer, and since the right people were notified and we got it fixed, I'm not gonna worry about it further

Comment by Marc Johnson [ 23/Apr/20 ]

Zak Burke

There is more than one possible answer, no single right answer, and since the right people were notified and we got it fixed, I'm not gonna worry about it further

Agreed, I removed a chunk of one of my replies because I didn't want to spend time on that topic again (we've been around that merry go round too many times recently)

Comment by Mike Taylor [ 23/Apr/20 ]

So just to clarify where we landed: Zak Burke you reassigned this to me. Does that mean it's on me to make the release of stripes-cli?

Comment by Zak Burke [ 23/Apr/20 ]

Nope; Ryan Berger is making the release. Sorry for the confusion.

I assigned it to you since you wrote the PR that fixes the actual bug in stripes-cli. I figured we'd close it once we verify it in the ui-tenant-settings build pipeline.

I could have written another Jira to cover the process of making the release so we could test it against ui-tenant-settings but that felt like overkill.

Comment by Mike Taylor [ 23/Apr/20 ]

Strong agreement that another issue would be overkill

I'm off to review Ryan Berger's PR.

Comment by Zak Burke [ 23/Apr/20 ]

Tested against ui-tenant-settings and its output is now correct for optionalOkapiInterfaces.

Comment by Mike Taylor [ 24/Apr/20 ]

Great news! Sorry for my initial screwup that made this necessary.

Generated at Thu Feb 08 23:21:37 UTC 2024 using Jira 1001.0.0-SNAPSHOT#100246-sha1:7a5c50119eb0633d306e14180817ddef5e80c75d.