MarquezProject / marquez-web

Marquez Web UI
22 stars 6 forks source link

Searches for key in all upper case #99

Closed frankcash closed 4 years ago

frankcash commented 4 years ago

Description

Tags are stored in all upper case https://github.com/MarquezProject/marquez/blob/6703d22be0833e5be9217e885e951876f4f13f40/src/main/java/marquez/service/models/Tag.java#L30 , thus it will be impossible for tags to ever be equal to a set of keys of ["is_pii", "is_compliant"] unless the values from the API are set to all lower or the keys are set to all upper case

works on #96

Checklist

Consider this image

Screen Shot 2020-05-06 at 3 24 59 PM
wslulciuc commented 4 years ago

@frankcash Thanks for the PR! For context, tags are configured for Marquez in config.yml. Tags are also uppercase to avoid cases like PII, PiI, Pii, etc. But it might be worth revisiting valid tag name rules applied on the backend such as acceptable characters, all lowercase vs uppercase, separated by underscores (_) etc. What are your thoughts?

frankcash commented 4 years ago

Yes, I was trying to enable the is_pii tag, to enable the lock icon and I had initialized it in the config file in all lower case.

I do agree it would be nice to reconsider the rules and allow upper and lower, but in the short term this fix this disparity.

On Wed, May 6, 2020 at 10:06 PM Willy Lulciuc notifications@github.com wrote:

@frankcash https://github.com/frankcash Thanks for the PR! For context, tags are configured for Marquez in config.yml https://github.com/MarquezProject/marquez/blob/master/config.example.yml#L71. Tags are also uppercase to avoid cases like PII, PiI, Pii, etc. But it might be worth revisiting valid tag name rules applied on the backend such as acceptable characters, all lowercase vs uppercase, separated by underscores (_) etc. What are your thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MarquezProject/marquez-web/pull/99#issuecomment-624986401, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABN32R6NEFTHHOO76O6HRC3RQIJQRANCNFSM4M2W3YTQ .

-- Charles Frank Cash FOSS Programmer cashc@acm.org https://github.com/frankcash https://keybase.io/frankcash https://github.com/frankcash

frankcash commented 4 years ago

Yeah, we could change the keys (as you suggest is_pii to PII) for the icons in the tag-to-badge component, but if we don’t want to do that right now this will fix it.

I’m okay with fixing it that way, I’d rather it just look for PII and COMPLIANT. I won’t be able to get a change in for that until tomorrow though (I am Eastern Time).

Our use case right now is just presenting this as a demo with our ETLs and tagging for PII is a big win, which is why I’m a bit eager to get this working.

On Wed, May 6, 2020 at 10:18 PM Willy Lulciuc notifications@github.com wrote:

@wslulciuc commented on this pull request.

I think we'll want to change the tags to uppercase in tag-to-badge.tsx https://github.com/MarquezProject/marquez-web/blob/master/src/config/tag-to-badge.tsx#L13? I'm also wondering if we'll need to update the tag mappings?

is_pii -> PII is_compliant -> COMPLIANT

Note that the COMPLIANT tag was very experimental at WeWork. It basically signified that a given dataset had:

  1. An owner
  2. A schema
  3. Data lineage
  4. Data freshness (SLA)

Also, data compliancy differs from org-to-org and it could be a combination of things (logic that may exist on the backend). Let me know if you have any thoughts on its usage.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MarquezProject/marquez-web/pull/99#pullrequestreview-407107214, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABN32R7C5GSRB3NMLMZV4U3RQIK5ZANCNFSM4M2W3YTQ .

-- Charles Frank Cash FOSS Programmer cashc@acm.org https://github.com/frankcash https://keybase.io/frankcash https://github.com/frankcash

frankcash commented 4 years ago

Obviously prioritizing the most correct fix is important!

On Wed, May 6, 2020 at 10:23 PM Frank Cash cash.franky@gmail.com wrote:

Yeah, we could change the keys (as you suggest is_pii to PII) for the icons in the tag-to-badge component, but if we don’t want to do that right now this will fix it.

I’m okay with fixing it that way, I’d rather it just look for PII and COMPLIANT. I won’t be able to get a change in for that until tomorrow though (I am Eastern Time).

Our use case right now is just presenting this as a demo with our ETLs and tagging for PII is a big win, which is why I’m a bit eager to get this working.

On Wed, May 6, 2020 at 10:18 PM Willy Lulciuc notifications@github.com wrote:

@wslulciuc commented on this pull request.

I think we'll want to change the tags to uppercase in tag-to-badge.tsx https://github.com/MarquezProject/marquez-web/blob/master/src/config/tag-to-badge.tsx#L13? I'm also wondering if we'll need to update the tag mappings?

is_pii -> PII is_compliant -> COMPLIANT

Note that the COMPLIANT tag was very experimental at WeWork. It basically signified that a given dataset had:

  1. An owner
  2. A schema
  3. Data lineage
  4. Data freshness (SLA)

Also, data compliancy differs from org-to-org and it could be a combination of things (logic that may exist on the backend). Let me know if you have any thoughts on its usage.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MarquezProject/marquez-web/pull/99#pullrequestreview-407107214, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABN32R7C5GSRB3NMLMZV4U3RQIK5ZANCNFSM4M2W3YTQ .

-- Charles Frank Cash FOSS Programmer cashc@acm.org https://github.com/frankcash https://keybase.io/frankcash https://github.com/frankcash

-- Charles Frank Cash FOSS Programmer cashc@acm.org https://github.com/frankcash https://keybase.io/frankcash https://github.com/frankcash

wslulciuc commented 4 years ago

@frankcash Totally makes sense. I'll merge the fix given this now aligns correctly with the backend logic. And I'll open an issue to account for subsequent work on tagging!

frankcash commented 4 years ago

Perfect! I can look into taking that ticket in the future. I also wonder if there is documentation that could be done somewhere to highlight these “fun tidbits.” 😂

On Wed, May 6, 2020 at 10:28 PM Willy Lulciuc notifications@github.com wrote:

@frankcash https://github.com/frankcash Totally makes sense. I'll merge the fix given this now aligns correctly with the backend logic. And I'll open an issue to account for subsequent work on tagging!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MarquezProject/marquez-web/pull/99#issuecomment-624992277, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABN32R4F7RPPMKVGGCI6OIDRQIMDFANCNFSM4M2W3YTQ .

-- Charles Frank Cash FOSS Programmer cashc@acm.org https://github.com/frankcash https://keybase.io/frankcash https://github.com/frankcash