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

Support import gzip json file #547

Closed
wants to merge 31 commits into from

Conversation

ndinh215
Copy link
Contributor

@ndinh215 ndinh215 commented Jan 8, 2016

Please review my implementation of supporting gzip file.

…'if-not-exists' in 'ongr:es:index:create' command
…'if-not-exists' in 'ongr:es:index:create' command
… of testcases to avoid impacting on each other.
@saimaz saimaz added the qa label Jan 8, 2016
@saimaz
Copy link
Member

saimaz commented Jan 8, 2016

First of all rebase all merge type commits. Let's try to keep a nice git history.

@saimaz
Copy link
Member

saimaz commented Jan 8, 2016

You forgot to add close comment for the specific issue.

@@ -38,7 +38,13 @@ protected function configure()
InputOption::VALUE_NONE,
'If the time suffix is used, its nice to create an alias to the configured index name.'
)
->addOption('no-mapping', null, InputOption::VALUE_NONE, 'Do not include mapping');
->addOption('no-mapping', 'nm', InputOption::VALUE_NONE, 'Do not include mapping')
Copy link
Member

Choose a reason for hiding this comment

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

Options shortcut should have not more than one letter. In this particular case let's leave this option without shortcut.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry for the comment, this commit is not related to this PR. Isn't it. There is something messed up with your git history...

@ndinh215
Copy link
Contributor Author

ndinh215 commented Jan 8, 2016

Sorry @saimaz , I will check again coveralls and add 'close issue'.

@saimaz
Copy link
Member

saimaz commented Jan 8, 2016

Please do a rebase. Now it's somehow combined with other PR's.

@ndinh215
Copy link
Contributor Author

ndinh215 commented Jan 8, 2016

Got it, @saimaz

@ndinh215
Copy link
Contributor Author

ndinh215 commented Jan 8, 2016

I will check again all, and inform to you on finish. Thanks.

@saimaz
Copy link
Member

saimaz commented Jan 8, 2016

No idea what you are doing here, but here's the instruction https://akrabat.com/the-beginners-guide-to-rebasing-your-pr/

@ndinh215
Copy link
Contributor Author

ndinh215 commented Jan 8, 2016

Yup, I did it. I included 2 implementations of the issue #423 & #382 to this PR (I will close the other PR related to #423 later).
@mvar is checking again the issue:

  • No need to create index after calling $manager = $this->getManager(); in CreateIndexCommandTest class.
  • Now, I still add the code check this:
if (!$manager->indexExists()) {
      $manager->createIndex();
 }

In the test class, the orders of test cases will impact on each other. Data was born in previous test cases could affect on later test cases.

@saimaz saimaz closed this Jan 8, 2016
@saimaz saimaz removed the qa label Jan 8, 2016
@saimaz
Copy link
Member

saimaz commented Jan 8, 2016

Please open a new well-formed PR with appropriate changes.

@ndinh215
Copy link
Contributor Author

ndinh215 commented Jan 8, 2016

Yup, I will do it. Thanks @saimaz .

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.

2 participants