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

feature/swagger-cli-version #43

Merged
merged 28 commits into from
Jul 18, 2019
Merged

feature/swagger-cli-version #43

merged 28 commits into from
Jul 18, 2019

Conversation

gratcliff
Copy link
Member

@gratcliff gratcliff commented Jul 12, 2019

This is half of a 2 part PR. The other half can be referenced in https://github.com/readmeio/readme/pull/ under the same name. You will need to have both branches running for full functionality.

This creates CLI interactions for the new version CRUD API. Additionally, it allows version control with the rdme swagger command.

1. Testbed covering individual version files
2. Major swagger testbed updates - WIP
1. Copy edit
2. QOL with main version interaction
3. Test case update
Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

Can you toss a .then() on this to catch these new API call successes and display a message? The API responses from versions:create and versions:delete are getting dumped to the terminal.

lib/versions/index.js Show resolved Hide resolved
@gratcliff
Copy link
Member Author

Can you toss a .then() on this to catch these new API call successes and display a message? The API responses from versions:create and versions:delete are getting dumped to the terminal.

Happy to.

@dok
Copy link

dok commented Jul 16, 2019

When I try to run NODE_ENV=localhost ./rdme.js versions:update --key=... --version=1.0 on an existing version, I'm getting a Cannot find target version message. I'm seeing in the readme side that the .0 in the 1.0 version semantic string is being stripped out.

Screen Shot 2019-07-16 at 11 00 10 AM
Screen Shot 2019-07-16 at 11 02 11 AM

Same thing happens with version:versionId the .0 suffix is being stripped.

lib/versions/create.js Outdated Show resolved Hide resolved
lib/versions/update.js Show resolved Hide resolved
README.md Show resolved Hide resolved
1. Error handling for dup creation
2. Help Params for versions
3. Copy
@gratcliff
Copy link
Member Author

When I try to run NODE_ENV=localhost ./rdme.js versions:update --key=... --version=1.0 on an existing version, I'm getting a Cannot find target version message. I'm seeing in the readme side that the .0 in the 1.0 version semantic string is being stripped out.

Screen Shot 2019-07-16 at 11 00 10 AM
Screen Shot 2019-07-16 at 11 02 11 AM

Same thing happens with version:versionId the .0 suffix is being stripped.

This was an issue with the minimist package converting anything x.0 to a number with parseInt. Fixed via config.

@gratcliff gratcliff requested review from erunion and dok July 16, 2019 22:36
@mjcuva
Copy link
Member

mjcuva commented Jul 16, 2019

Few things I noticed!

  • We can probably break up the help a little bit better, maybe have separate sections for versions, docs, and swagger stuff? Also fixing the space so it's easier to read (it's nice having the second column lined up)

image

  • When I tried syncing a swagger file with version 1.0.0, it said there was no matching version since the version in readme was 1.0. I think if we convert the semver version in the swagger file to valid semver (semver.clean) and compare it with version_clean in the db that will fix it!

@erunion
Copy link
Member

erunion commented Jul 16, 2019

We can probably break up the help a little bit better, maybe have separate sections for versions, docs, and swagger stuff? Also fixing the space so it's easier to read (it's nice having the second column lined up)

I'll factor this into #45

@gratcliff
Copy link
Member Author

Few things I noticed!

  • We can probably break up the help a little bit better, maybe have separate sections for versions, docs, and swagger stuff? Also fixing the space so it's easier to read (it's nice having the second column lined up)
image
  • When I tried syncing a swagger file with version 1.0.0, it said there was no matching version since the version in readme was 1.0. I think if we convert the semver version in the swagger file to valid semver (semver.clean) and compare it with version_clean in the db that will fix it!

Good call, checked the functionality in the frontend and that maps. I'll make the change (although it'll be in the other repo).

@mjcuva
Copy link
Member

mjcuva commented Jul 16, 2019

We should always do the prompt for which version to upload the swagger file to (unless they pass --version in the command, or have the --id already in there). Currently if the version already exists in readme, it will just upload it to that version. This can be kinda confusing, so I think it's better to be explicit!

@gratcliff
Copy link
Member Author

We should always do the prompt for which version to upload the swagger file to (unless they pass --version) in the command. Currently if the version already exists in readme, it will just upload it to that version

This seems to be working as intended. Why put them through the process if it works behind the scenes for the user?

@dok
Copy link

dok commented Jul 17, 2019

I can't seem to update the version if it is a main version. Am I doing something weird here?

Screen Shot 2019-07-17 at 11 55 25 AM

It would be helpful to know why it's breaking in this stage

@gratcliff
Copy link
Member Author

I can't seem to update the version if it is a main version. Am I doing something weird here?

Screen Shot 2019-07-17 at 11 55 25 AM

It would be helpful to know why it's breaking in this stage

This has been rectified :)

@mjcuva
Copy link
Member

mjcuva commented Jul 18, 2019

When I run it locally nothing happens and it doesn't output anyting. Seems like api/v1/version is returning a 404?

image

@mjcuva
Copy link
Member

mjcuva commented Jul 18, 2019

JK I forgot to checkout the branch in readme!

Looks good for importing a new swagger file, but when I pass in --id it still asks me for the version, which shouldn't happen since we already know what version the swagger file was synced with. In this case we can just update the existing one!

image

Copy link
Member

@mjcuva mjcuva left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@gratcliff gratcliff merged commit 3d56c4f into master Jul 18, 2019
@erunion erunion deleted the feature/cli-swagger-version branch July 19, 2019 17:52
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.

5 participants