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

Improve table & columns description formatting (#98) #79

Merged

Conversation

Mikhail-Ivanov
Copy link
Contributor

@Mikhail-Ivanov Mikhail-Ivanov commented Sep 19, 2019

Summary of Changes

Introduced support of Markdown descriptions for tables & columns.
This module changes are intended to pass description text as a request body (instead of using URI parameter).

Related changes at amundsenfrontendlibrary.

Tests

Unit tests for table and column description upsertion have been modified

Documentation

Markdown description format demo:
https://rexxars.github.io/react-markdown/

@feng-tao
Copy link
Member

Seems pretty safe. In the mean time, could you also fix the CLA check? Normally it is due to you didn't configure your github handle(username, email) in your local box.
cc @jinhyukchang

"""
try:
description = self.parser.parse_args()['description']
Copy link
Member

Choose a reason for hiding this comment

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

@verdan , I assume it won't affect the atlas backend?

Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't. go ahead!!

@feng-tao
Copy link
Member

the backend code looks fine. So once we are agree with the FE part, we are good to go with this pr. And please increase the version as well.

@Mikhail-Ivanov Mikhail-Ivanov force-pushed the feature/table_description_format branch from db42b03 to 4e869b7 Compare September 20, 2019 10:51
@codecov-io
Copy link

codecov-io commented Sep 20, 2019

Codecov Report

Merging #79 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage    74.8%   74.85%   +0.04%     
==========================================
  Files          19       19              
  Lines        1008     1010       +2     
  Branches       88       88              
==========================================
+ Hits          754      756       +2     
  Misses        231      231              
  Partials       23       23
Impacted Files Coverage Δ
metadata_service/api/table.py 48.05% <100%> (+0.68%) ⬆️
metadata_service/api/column.py 60.71% <100%> (+1.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5db116b...fb02351. Read the comment docs.

@Mikhail-Ivanov
Copy link
Contributor Author

Please note that I've got rid of reqparse because of this (also faced issues while writing unit tests):
https://stackoverflow.com/questions/19384526/how-to-parse-the-post-argument-to-a-rest-service

"""
try:
description = json.loads(request.json).get('description')
Copy link
Contributor

@ttannis ttannis Sep 23, 2019

Choose a reason for hiding this comment

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

What do you all think about having a check for what happens if the description is None? Does self.client.put_column_description end up handling this (in that case no need for a check here), or will the description be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked it locally (via Postman):

  1. {"description": ""} - this request payload will truncate description
  2. request payload with no "description" tag ends with {"msg": "Encountered exception: None"} and description data stay untouched

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming the behavior for empty string and None. They behave as we expect, so no changes would be needed.

@feng-tao feng-tao merged commit a4ae703 into amundsen-io:master Sep 25, 2019
@Mikhail-Ivanov Mikhail-Ivanov deleted the feature/table_description_format branch September 26, 2019 06:38
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