Skip to content

Activedocs CRUD operations - #145

Merged
miguelsorianod merged 4 commits into
masterfrom
activedocs-subcommand
May 22, 2019
Merged

Activedocs CRUD operations#145
miguelsorianod merged 4 commits into
masterfrom
activedocs-subcommand

Conversation

@miguelsorianod

@miguelsorianod miguelsorianod commented May 21, 2019 •
edited by eguzki
Loading

Copy link
Copy Markdown
Contributor

Fixes #125

@miguelsorianod
miguelsorianod requested a review from eguzki May 21, 2019 10:36
@miguelsorianod miguelsorianod changed the title Activedocs CRUD operations [WIP] Activedocs CRUD operations May 21, 2019
@miguelsorianod

miguelsorianod commented May 21, 2019

Copy link
Copy Markdown
Contributor Author

Added ActiveDocs Entity

@miguelsorianod
miguelsorianod force-pushed the activedocs-subcommand branch from 0294b52 to 637c8f5 Compare May 21, 2019 10:48
eguzki
eguzki reviewed May 21, 2019
View reviewed changes
Comment thread lib/3scale_toolbox/commands/active_docs_command.rb Outdated
@miguelsorianod
miguelsorianod force-pushed the activedocs-subcommand branch 3 times, most recently from 9aec7da to 421a261 Compare May 21, 2019 15:35
@miguelsorianod

miguelsorianod commented May 21, 2019

Copy link
Copy Markdown
Contributor Author

I've updated the PR to add:

  • Implementation of create, delete and list
  • Unit tests
  • Integration tests

@miguelsorianod
miguelsorianod force-pushed the activedocs-subcommand branch from 2c4b5f5 to 2e365e5 Compare May 21, 2019 16:16
@miguelsorianod

miguelsorianod commented May 21, 2019

Copy link
Copy Markdown
Contributor Author

The create command usage is:

USAGE
    3scale activedocs activedocs create <remote> <activedocs-name> <spec>

For the moment I've not implemented the 'apply' command because I've not found a suitable format for the command:

  • If we make a positional parameter it will mean is mandatory. However, when performing the update part of the apply that is optional, so it shouldn't be mandatory
  • if we make an optional parameter, it will mean is optional. However, when performing the create part of the apply that is mandatory, so it shouldn't be mandatory

@miguelsorianod
miguelsorianod requested a review from eguzki May 21, 2019 16:25
@eguzki

eguzki commented May 21, 2019

Copy link
Copy Markdown
Member

I think apply can be useful. For instance to publish or hide it. And OAS content should be optional. When executing apply command and the toolbox has to create because it does not exist, then optional parameter should exist otherwise, command should fail.

@miguelsorianod
miguelsorianod force-pushed the activedocs-subcommand branch 2 times, most recently from 77dd599 to a5449a8 Compare May 22, 2019 13:08
@miguelsorianod miguelsorianod changed the title [WIP] Activedocs CRUD operations Activedocs CRUD operations May 22, 2019
@miguelsorianod

miguelsorianod commented May 22, 2019

Copy link
Copy Markdown
Contributor Author

Implemented apply and added integration tests.

I think this should be finished. @eguzki could you review it?

Thank you.

eguzki
eguzki requested changes May 22, 2019
View reviewed changes
Comment thread docs/activedocs.md Outdated
Comment thread docs/activedocs.md
Comment thread docs/activedocs.md Outdated
Comment thread docs/activedocs.md Outdated
Comment thread docs/activedocs.md
Toolbox will figure it out.
* Update to `published=true` activedocs by `--publish` flag.
* Update to `published=false` method by `--hide` flag.
* *The `--openapi-spec` flag is mandatory when the specified activedocs

@eguzki eguzki May 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it is not a flag, but optional parameter

Comment thread docs/activedocs.md Outdated
Comment thread docs/activedocs.md
DESCRIPTION
List all defined ActiveDocs

OPTIONS FOR ACTIVEDOCS

@eguzki eguzki May 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It could be useful --service to list only activedocs for a certain service. But let's users demand first

Comment thread lib/3scale_toolbox/commands/activedocs_command/apply_command.rb
Comment thread spec/integration/commands/activedocs_command/apply_command_spec.rb Outdated
include_context :random_name
include_context :resources
subject { ThreeScaleToolbox::CLI.run(command_line_str.split) }
let(:remote) do

@eguzki eguzki May 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let(:remote) { client_url } should work

@miguelsorianod miguelsorianod May 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

Comment thread lib/3scale_toolbox/commands/activedocs_command/apply_command.rb
@miguelsorianod
miguelsorianod force-pushed the activedocs-subcommand branch from a5449a8 to a40237e Compare May 22, 2019 16:26
@miguelsorianod

miguelsorianod commented May 22, 2019

Copy link
Copy Markdown
Contributor Author

Applied requested changes

@miguelsorianod
miguelsorianod requested a review from eguzki May 22, 2019 16:27
@miguelsorianod
miguelsorianod merged commit 6f8cc14 into master May 22, 2019
@miguelsorianod
miguelsorianod deleted the activedocs-subcommand branch May 22, 2019 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eguzki eguzki eguzki approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

command to create/update/delete Activedocs

2 participants

@miguelsorianod @eguzki