Issue 147/multi doc pathways

[samshuster]

Summary of Changes

mr to get feedback as part of amundsen-io/amundsen#147
Currently blocked by amundsen-common being merged and getting feedback.

Here is current screenshot. You can see the added sections in bottom left.

Current features:

• supports 0 to n separate programmatic sections
• supports markdown
• does not support editing
• does not currently support search, but very easy to add that
• supports configurable display names for the sections as well as order

Tests

Updated python api tests for configuring the order of the sections
Updated the javascript tests to the best of my ability, but I believe more tests could be used here.

Documentation

Updated some documentation on how to configure these sections

CheckList

Make sure you have checked all steps below to ensure a timely review.

• PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
• PR includes a summary of changes, including screenshots of any UI changes.
• PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public python functions and the classes in the PR contain docstrings that explain what it does
• PR passes all tests documented in the developer guide

[codecov-io]

Codecov Report

Merging #381 into master will decrease coverage by 0.02%.
The diff coverage is 76.66%.

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
- Coverage   78.33%   78.30%   -0.03%     
==========================================
  Files         194      194              
  Lines        4819     4845      +26     
  Branches      625      633       +8     
==========================================
+ Hits         3775     3794      +19     
- Misses        998     1003       +5     
- Partials       46       48       +2     

Impacted Files	Coverage Δ
...ication/static/js/components/TableDetail/index.tsx	0.00% <0.00%> (ø)
...plication/static/js/ducks/tableMetadata/reducer.ts	88.88% <ø> (ø)
...dsen_application/static/js/fixtures/globalState.ts	100.00% <ø> (ø)
amundsen_application/api/utils/metadata_utils.py	79.31% <80.00%> (-0.18%)	⬇️
amundsen_application/config.py	100.00% <100.00%> (ø)
...s/components/TableDetail/EditableSection/index.tsx	100.00% <100.00%> (ø)
...ication/static/js/components/common/Flag/index.tsx	100.00% <100.00%> (ø)
amundsen_application/api/utils/search_utils.py	96.00% <0.00%> (ø)
...dsen_application/static/js/interfaces/Resources.ts	100.00% <0.00%> (ø)
...ts/common/ResourceListItem/TableListItem/index.tsx	100.00% <0.00%> (ø)

---

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 2f7930d...b50c038. Read the comment docs.

[samshuster]

ok @ttannis this is ready for you to review!

[jornh]

Would be good to mention PROGRAMMATIC_DISPLAY config in https://github.com/lyft/amundsenfrontendlibrary/pull/381/files#diff-da4c534f10ac33888daef3b30ba35474 In the Flask config documentation: https://github.com/lyft/amundsenfrontendlibrary/blob/master/docs/flask_config.md

Also, maybe worth showcasing the demo screenshots above in the same document? I’d prefer that over keeping adding optional customization screenshots to the main README (otherwise it’s just getting longer and longer)

Finally maybe also worth to point to how to ingest data as introduced in databuilder amundsen-io/amundsendatabuilder#187?

[samshuster]

ok @jornh documentation for frontend added. I will go back and add in some documentation to databuilder too.

[jornh]

Overall great write up! Thanks @samshuster for all your effort on this great feature.

One clarifying question, does the sentence I marked below imply that:

If PROGRAMMATIC_DISPLAY is left at None all added fields are still showing up, so that display is entirely dynamically data-driven without configuration. Meaning configuration merely adds the (nice) benefit of setting display order.

If so, I’ll leave it to you if you think it’s worth to add this to the documentation...

Looking forward to see if there’s any Lyft designer team input 👍

[ttannis]

Hey @samshuster below is a screenshot for what our designer would like to see, and what you have is basically almost there. The design concepts we want to maintain are:

1. The programmatic descriptions will be in their own section which will live at the bottom left hand panel. The reason is that since this information can be any number of descriptions, we would not want them to push down all other metadata, especially things related to quality features.
2. This programmatic description section will have header as seen the mocks to make it clear the the information is read only and auto-generated.
3. Since we do not know what the data in these descriptions will be, each description will take up the full width of the panel.
4. Maintain existing section header styles, more specifically here the fact that we use "Title Case", which means s3 crawler header should be rendered as S3 Crawler

[ttannis]

Initial review. Let me know your thoughts and let's discuss any questions before moving forward.

[samshuster]

hey @ttannis thanks for the great feedback!
Will be going over this later today and tomorrow

[samshuster]

Ok @ttannis thank you so much for your feedback, learning a lot!

At this point I believe I am ready for another review. I have also made the design changes per the recommendations. Please find a screenshot of what it looks like now:

Per our long conversation about optional configuration. I have include the Flag convertText function by default.

[samshuster]

ok @ttannis it is ready for you to take another look at it.

[ttannis]

@samshuster looks good, a few more small comments. If any of the comments on the unit tests don't make sense just let me know, I can always address them after this is merged and you can have that PR for future reference.

[ttannis]

Instead of needing a new test and a new object, you could add programmatic_descriptions to self.mock_metadata and self.expected_parsed_metadata. Beyond that I think the new tests you'd have to write wouldn't be in this file.

The new functionality you added can be summarized as:

1. _marshall_table_full updates the programmatic_descriptions with _update_prog_descriptions
2. _update_prog_descriptions acts updates progrmmatic_descriptions according to certain criteria
3. _sort_prog_descriptions returns a order value according to certain criteria

Since the idea is to unit test new functionality I would expect to see tests for these new methods. However I notice we are lacking a test file for metadata_utils. Overall I'd say we could add programmatic_descriptions to the test input and output for test_get_table_metadata_success, but beyond that we would want to just make a test_metadata_utils.py in tests/unit/utils and add a test for those methods you added.

[samshuster]

Alright, here is what I did, hopefully this is pretty much in line with your recommendations:

• I added programmatic_descriptions to mock_data
• I removed the two separate programmatic_description tests from test_v0
• I added a new test_metadata_utils file and added a single test for marshall_table_full at this time. My thought is that by testing this method, it tests the other private methods.

[ttannis]

• My thought is that by testing this method, it tests the other private methods.

While that is true, it's a good practice for unit tests to isolate logic and explicitly test behaviors. For example if method A() calls method B() and returns some output C, we could call method A(), check that C is returned and consider both A() and B() tested.

However with this approach if the logic in B() ever changes then the test fails. Then the question becomes why should a change in the internal logic for B() be able to break a unit test for A()? We would address this by mocking B() with some fake logic in the unit test. I will tag you in a PR that demonstrates this.

Thanks for your work on this feature!

[ttannis]

Thanks @samhuster. We will merge this today and I'll make some tweaks to unit tests. This will be part of the next version of amundsenfrontendlibrary. You can expect it within the next week, unless we discover and major bugs with the other new features we've been testing internally.

[ttannis]

Trailing commas are okay in python and for a couple of nuanced reasons can be desirable. No need to put them back in (we should have a linter handle this at some point), but also no need to remove them in the future:
https://stackoverflow.com/questions/11597901/why-are-trailing-commas-allowed-in-a-list

[ttannis]

This isn't exactly what I meant but I can modify this after merging and tag you for reference. Ideally, we would have already had a test_metadata_utils set up with at least one example to make things easier.