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

Bugfix: allow None type for dashboard title #627

Conversation

d-swift
Copy link

@d-swift d-swift commented Aug 26, 2022

Change description

Closes #626 content validation bug where a dashboard have title=None. I suspect this is from lookml dashboards.

Type of change

  • Bug fix (fixes an issue)
  • New feature (adds functionality)

Related issues

Closes #626

Checklists

I was unable to run full test suite locally. I can run if you can share API credentials for your test Looker instance.

Security

  • Security impact of change has been considered
  • Code follows security best practices and guidelines

Code review

  • Pull request has a descriptive title and context useful to a reviewer

@d-swift d-swift changed the title Bugfix: allow none for dashboard title Bugfix: allow None type for dashboard title Aug 26, 2022
@DylanBaker
Copy link
Collaborator

DylanBaker commented Aug 26, 2022

Thanks @d-swift!

I think there's one additional place this needs to be addressed, but I'm not sure what the right way to do it it. The title is used in the print out of the results, so we'll need to defer to something else. Relevant code is here.

Is it at all clear why this content doesn't have a title in your instance? We could defer to something like tile_title, but I'd want to make sure that's present.

@joshtemple
Copy link
Collaborator

@DylanBaker, we relaxed the content type to allow all other possible content types. It's possible some of these just don't have titles (we would have been skipping them and warning previously).

@DylanBaker
Copy link
Collaborator

@joshtemple I think in that case it's possible the printing in the CLI and the reporting in the app might not be working for other content types. Is that possible?

@DylanBaker
Copy link
Collaborator

Closing this. Going to be covered in #651

@DylanBaker DylanBaker closed this Feb 24, 2023
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.

Unexpected content validation errors
3 participants