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

Dynamic OpenAPI UI #6209

Merged
merged 103 commits into from
Feb 14, 2019
Merged

Dynamic OpenAPI UI #6209

merged 103 commits into from
Feb 14, 2019

Conversation

madalynrose
Copy link
Contributor

pull just frontend commits from #6129

Madalyn Parker and others added 30 commits January 15, 2019 11:55
@madalynrose madalynrose changed the base branch from master to 1.1-beta February 12, 2019 20:32
Copy link
Contributor

@meirish meirish left a comment

Choose a reason for hiding this comment

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

Sorry, lots of little comments! This looks good, the main thing I am worried about is using pathForType for the place in the model that determines the OpenAPI url. Is there a way we can do that differently?

ui/app/adapters/auth-config/radius.js Outdated Show resolved Hide resolved
ui/app/models/role-pki.js Outdated Show resolved Hide resolved
ui/app/models/role-aws.js Outdated Show resolved Hide resolved
ui/app/components/auth-config-form/config.js Outdated Show resolved Hide resolved
ui/app/components/auth-config-form/options.js Outdated Show resolved Hide resolved
ui/app/services/path-help.js Outdated Show resolved Hide resolved
ui/app/utils/openapi-to-attrs.js Show resolved Hide resolved
details.type = 'number';
}
let editType = details.type;
if (details.format === 'seconds') {
Copy link
Contributor

Choose a reason for hiding this comment

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

will seconds always mean it's a ttl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was my understanding. @kalafut is this the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correction: seconds means it's a duration, meaning a ttl form field would make most sense

ui/tests/acceptance/secrets/backend/pki/cert-test.js Outdated Show resolved Hide resolved
ui/tests/pages/secrets/backend/pki/generate-cert.js Outdated Show resolved Hide resolved
//as determined by the expandOpenApiProps util
getProps(helpUrl, path, backend) {
return this.ajax(helpUrl, backend).then(help => {
let props = help.openapi.paths[path].post.requestBody.content['application/json'].schema.properties;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know here that there will always be one path (at least until we get to more complex stuff with CRUD views, etc)? if so, what do you think about doing paths[0] instead of paths[path] ? When looking through the other code I wasn't sure what path was in those objects, but if this is all we're using it for, maybe we don't need it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

paths is an object, so it's either this or doing something like

let path = Object.keys( help.openapi.paths)[0]
let props = paths[path].post.requestBody.content['application/json'].schema.properties;

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh yeah I didn't realize that, but yeah what do you think about getting the keys if there's only going to be one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done and done

Copy link
Contributor

@meirish meirish left a comment

Choose a reason for hiding this comment

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

Excited for this! May need to rebase to get the changelog changes out of there, but 👍 - are the go changes ready to merge too?

@madalynrose madalynrose merged commit 368d431 into 1.1-beta Feb 14, 2019
@madalynrose madalynrose deleted the openapi-models-ui branch February 14, 2019 18:52
{ default: ['organization'] },
{
'GitHub Options': ['baseUrl'],
},
];
if (this.newFields) {
groups = combineFieldGroups(groups, this.newFields, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

this combines the fields we have defined in the model with those returned from OpenAPI. new fields should not be added to the model -- ideally we should transition to only using fields from OpenAPI since it's meant to be the source of truth.


const { attr } = DS;

export default AuthConfig.extend({
useOpenAPI: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

whether or not this model class should be re-opened and extended with additional fields from OpenAPI.

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.

6 participants