Skip to content

Conversation

@0-sv
Copy link

@0-sv 0-sv commented May 26, 2021

Closes #169.

Please see and respond to my comments next to the code in the review window.

Note:

  • I moved settings.yaml to settingsModels.yaml because it is so tangled up with the JS (otomi-models.ts and all the code that uses the models) code that I didn't want to spend time right now to fix it... I can still do it if you want but it's gonna take some time.
  • I fixed the race condition in the npm scripts @j-zimnowoda was complaining about by re-ordering the run and run-p.
  • I added a command to remove vendors/openapi/otomi-api.json. There is an issue where sometimes there is a unsuccessful bundle/dereference that it doesn't replace the schema with a new one, so you would be assuming all the code you commit would generate but in reality it doesn't do anything. Now the developer should be more mindful about the contents of this file.
  • Like I said, oneOf as a direct subschema in oneOf is a known issue. This was NOT fixed in the previous iteration, merely moved. The limitation is ONLY present if you use it as a direct subschema, but if you place a root property and then place the oneOf, it is fine. It is a work-around and hopefully not the permanent solution. The payload will look like this:
{ 
  "settings":
    { "alerts": 
    ...
    }
}
  • I assumed schemas start with a capital letter (e.g. Alerts not alerts), but definitions don't start with a capital letter (alerts not Alerts).
  • See my comments on what the discriminator is actually useful for (tl;dr: performance). Not a silver bullet.
  • I haven't fully finished the form validation (e.g. with pattern) because I want to submit this for review now.
  • Generated model end result:
/**
 * The otomi-stack API
 * Holds the entire schema needed by console to autogenerate forms
 *
 * The version of the OpenAPI document: 0.0.0-PLACEHOLDER
 * 
 *
 * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
 * https://openapi-generator.tech
 * Do not edit the class manually.
 */

import { RequestFile } from './models';
import { Alerts } from './alerts';
import { Azure } from './azure';
import { DNS } from './dNS';
import { KMS } from './kMS';
import { OIDC } from './oIDC';
import { Otomi } from './otomi';
import { SMTP } from './sMTP';

export class InlineObject {
    'settings': Alerts | Azure | DNS | KMS | OIDC | Otomi | SMTP;

    static discriminator: string | undefined = undefined;

    static attributeTypeMap: Array<{name: string, baseName: string, type: string}> = [
        {
            "name": "settings",
            "baseName": "settings",
            "type": "Alerts | Azure | DNS | KMS | OIDC | Otomi | SMTP"
        }    ];

    static getAttributeTypeMap() {
        return InlineObject.attributeTypeMap;
    }
}

@0-sv 0-sv requested review from Morriz and j-zimnowoda May 26, 2021 09:33
Copy link
Contributor

@j-zimnowoda j-zimnowoda left a comment

Choose a reason for hiding this comment

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

Thanks for explaining your reasoning in the PR overview, as it helped me to understand your decisions.

Copy link
Contributor

@Morriz Morriz left a comment

Choose a reason for hiding this comment

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

Ok, we are learning how to do this it seems?
But I get a headache now, seeing models duplicated all over the place and having different approaches and conventions introduced. I understand there might be a need to add all schemas under api.yaml's schemas, using lowercase for subschemas, and root models in CamelCase. Ok. But if we now know how the wiring works, can we please please please remove all the aberrations that are still there? The duplicated content etc? That historic workaround crap...

@0-sv 0-sv requested a review from Morriz June 1, 2021 09:45
@0-sv 0-sv self-assigned this Jun 1, 2021
@0-sv 0-sv added the Task Scrum task label Jun 1, 2021
@0-sv 0-sv force-pushed the task/169/sub-setting-endpoints branch from 420c057 to 4f8a936 Compare June 1, 2021 10:45
@0-sv
Copy link
Author

0-sv commented Jun 2, 2021

@j-zimnowoda approved the schema, so I can merge the logic into this pull request (as discussed), after addressing RBAC.

Copy link
Contributor

@j-zimnowoda j-zimnowoda left a comment

Choose a reason for hiding this comment

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

Please, implement endpoints and we are done with otomi-api part

Copy link
Contributor

@Morriz Morriz left a comment

Choose a reason for hiding this comment

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

almost there...can we please keep the typings, it is not so hard to do so

@0-sv
Copy link
Author

0-sv commented Jun 2, 2021

I have a couple more things on my to-do:

  • Enrich API auth tests
  • Test read/write of otomi-values
  • Test read/write database state when making requests to endpoints
  • Enrich models where relevant for this pr
  • Add back DNS to session

Copy link
Contributor

@Morriz Morriz left a comment

Choose a reason for hiding this comment

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

I see only one space diff and it seems I am asked for a review? Did you ask me to review again or is github weird?

I ask this, because I just asked for some typing changes, but don't see them yet....

@0-sv
Copy link
Author

0-sv commented Jun 2, 2021

I did not ask for a review, I just pushed my latest changes. There is no need currently for another review.

Copy link
Contributor

@j-zimnowoda j-zimnowoda left a comment

Choose a reason for hiding this comment

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

Just few remarks.

@0-sv 0-sv force-pushed the task/169/sub-setting-endpoints branch from 8098015 to 2c1a9f5 Compare June 8, 2021 14:45
@0-sv 0-sv closed this Jun 8, 2021
@0-sv 0-sv deleted the task/169/sub-setting-endpoints branch June 8, 2021 14:46
@0-sv 0-sv reopened this Jun 9, 2021
@0-sv 0-sv requested a review from Morriz June 9, 2021 12:59
@0-sv
Copy link
Author

0-sv commented Jun 9, 2021

Closes #175.

Copy link
Contributor

@j-zimnowoda j-zimnowoda left a comment

Choose a reason for hiding this comment

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

You are great

Co-authored-by: Sebastiaan Verbeek <sebastiaan.verbeek@redkubes.com>
token:
type: string
type: object
vault:
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor

@Morriz Morriz left a comment

Choose a reason for hiding this comment

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

one inconsistency detected with crds

@0-sv 0-sv force-pushed the task/169/sub-setting-endpoints branch from c511364 to edc145c Compare June 10, 2021 13:35
@0-sv 0-sv requested a review from Morriz June 10, 2021 14:20
@Morriz Morriz merged commit 195b94d into master Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Task Scrum task

Projects

None yet

4 participants