Skip to content

Add explicit main_configs, user_configs and dictionaries in integration tests#13647

Merged
vitlibar merged 34 commits intoClickHouse:masterfrom
qoega:integration-explicit-configs
Sep 4, 2020
Merged

Add explicit main_configs, user_configs and dictionaries in integration tests#13647
vitlibar merged 34 commits intoClickHouse:masterfrom
qoega:integration-explicit-configs

Conversation

@qoega
Copy link
Copy Markdown
Member

@qoega qoega commented Aug 12, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Build/Testing/Packaging Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Integration tests use default base config. All config changes are explicit with main_configs, user_configs and dictionaries parameters for instance.

Detailed description / Documentation draft:

Splitted a part of large PR #10484 to separate one

By adding documentation, you'll allow users to try your new feature immediately, not when someone else will have time to document it later. Documentation is necessary for all features that affect user experience in any way. You can add brief documentation draft above, or add documentation right into your patch as Markdown files in docs folder.

If you are doing this for the first time, it's recommended to read the lightweight Contributing to ClickHouse Documentation guide first.

@robot-clickhouse robot-clickhouse added the pr-build Pull request with build/testing/packaging improvement label Aug 12, 2020
@qoega qoega marked this pull request as draft August 12, 2020 11:51
@qoega qoega marked this pull request as ready for review August 13, 2020 02:20
node3 = cluster.add_instance('node3', config_dir="configs", main_configs=['configs/remote_servers.xml', 'configs/log_conf.xml'], with_zookeeper=True)
node4 = cluster.add_instance('node4', config_dir="configs", main_configs=['configs/remote_servers.xml', 'configs/log_conf.xml'], with_zookeeper=True)
node5 = cluster.add_instance('node5', config_dir="configs", main_configs=['configs/remote_servers.xml', 'configs/log_conf.xml'], with_zookeeper=True)
node3 = cluster.add_instance('node3', user_configs=[], main_configs=['configs/remote_servers.xml', 'configs/log_conf.xml'], with_zookeeper=True)
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.

user_configs=[] is default value, isn't it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. Leftovers after sed.

Copy link
Copy Markdown
Member

@vitlibar vitlibar Sep 4, 2020

Choose a reason for hiding this comment

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

Then could you remove user_configs=[] here and in other places where cluster.add_instance() is called?

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.

And main_configs=[] as well. Just to make the code shorter.

@vitlibar vitlibar merged commit 81db73f into ClickHouse:master Sep 4, 2020
self.ignore_error = ignore_error

#print " ".join(command)
print " ".join(command)
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.

Why did you uncomment this line and other print lines?

</structure>
</dictionary>
</dictionaries>
</yandex> No newline at end of file
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.

Minor: it's better to add an empty line at the end of the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-build Pull request with build/testing/packaging improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants