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

Elide namespace support #1509

Merged
merged 20 commits into from
Nov 11, 2021
Merged

Elide namespace support #1509

merged 20 commits into from
Nov 11, 2021

Conversation

daniellevinson
Copy link
Member

@daniellevinson daniellevinson commented Oct 29, 2021

Description

Add support for Elide namespaces https://elide.io/pages/guide/v6/04-analytics.html#namespaces

Proposed Changes

WS:

  • add optional namespaces list to the DataSource model
  • add a namespace to datasources config
  • new Hjson config file for DemoNamespace and a new table TrendingNow that belongs to that namespace (same physical base table)

UI:

  • Add a namespace filter to the GraphQL metadata queries GetTables and GetTable
  • add the namespace connection to the schema and table fragment
  • normalizer: use friendlyName as name
  • Show an ellipsis at the front of the column id so table namespace prefix is hidden

Screenshots

Screen Shot 2021-11-01 at 11 10 18 AM

Screen Shot 2021-11-01 at 11 10 11 AM

Screen Shot 2021-11-01 at 11 10 33 AM

License

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@daniellevinson daniellevinson marked this pull request as ready for review November 1, 2021 18:56
Copy link
Member

@kevinhinterlong kevinhinterlong left a comment

Choose a reason for hiding this comment

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

It would be nice to have more of a service around dataSources instead of relying directly on config.navi.dataSources. Then we could do stuff like fetch all the namespaces from an elide datasource automatically rather than configuring each namespace manually

@@ -23,6 +23,14 @@
"uri": "http://localhost:8080/graphql/api/v1",
"type": "elide",
"suggestedDataTables": []
},
Copy link
Contributor

@jkusa jkusa Nov 2, 2021

Choose a reason for hiding this comment

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

Can we model namespaces like this:

      {
        "name": "corpAnalytics",
        "type": "elide",
        "uri": "http://localhost:8080/graphql/api/v1",
        "displayName": "Corp Analytics - General",  // for default namespace
        "suggestedDataTables": [] // for default namespace
        "namespaces": [{
          "name": "rev",
          "displayName": "Revenue",
          "suggestedDataTables": ["allRevenue"]
        }]        
      },

Then the datasource id for namespaces can be something like:

  • corpAnalytics.default
  • corpAnalytics.rev

@daniellevinson
Copy link
Member Author

daniellevinson commented Nov 2, 2021

It would be nice to have more of a service around dataSources instead of relying directly on config.navi.dataSources. Then we could do stuff like fetch all the namespaces from an elide datasource automatically rather than configuring each namespace manually

Yeah @jkusa had similar feedback (in addition to his comment above) and said we can address that in a separate PR. I thought we wanted to explicitly configure which namespaces we want to use.

throw new Error(`Datasource ${dataSourceName} should be configured in the navi environment`);
if (lookupNamespace) {
const namespace = dataSource.namespaces?.find(({ name }) => name === lookupNamespace);
if (!namespace) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we store elideOne.default as a data source?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, just elideOne

Copy link
Member

@kevinhinterlong kevinhinterlong left a comment

Choose a reason for hiding this comment

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

Just the one comment

uri: string;
options?: Options;
namespaces?: DataSourceNamespace[];
suggestedDataTables?: string[];
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move suggestedDataTables to DataSourceNamespace?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
suggestedDataTables?: string[];

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I did initially, I just bumped it to the root level because in the future we're not going to have namespaces at all and we'll have to somehow define all suggested tables at the root level
But yeah maybe for now it makes more sense to have it in DataSourceNamespace

return yield sortBy(sources, ['name']);
}

@task *fetchDataSourceNamespaces(dataSource: NaviDataSource): TaskGenerator<SourceItem<NaviDataSource>[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Long term it would be good to have this layer not know about namespaces. We can just process the original data sources with nested namespaces and generate a single flat list of data sources.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, like you and @kevinhinterlong suggested we should maybe have a service around that to discover namespaces and flatten

packages/data/config/navi-config.d.ts Show resolved Hide resolved
uri: string;
options?: Options;
namespaces?: DataSourceNamespace[];
suggestedDataTables?: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
suggestedDataTables?: string[];

Copy link
Contributor

@jkusa jkusa left a comment

Choose a reason for hiding this comment

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

Just the one comment otherwise 👍

@daniellevinson daniellevinson merged commit 04e324c into master Nov 11, 2021
@daniellevinson daniellevinson deleted the elide-namespace branch November 11, 2021 18:07
@jkusa jkusa mentioned this pull request Jan 11, 2022
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.

None yet

3 participants