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

Distinguish between regular and extended app describe #298

Merged
merged 3 commits into from
Apr 2, 2022

Conversation

janelletavares
Copy link
Contributor

@janelletavares janelletavares commented Apr 1, 2022

Description of change

Fixes https://github.com/meroxa/turbine-project/issues/111

Type of change

  • New feature
  • Bug fix
  • Refactor
  • Documentation

How was this tested?

  • Unit Tests
  • Tested in staging

Demo

Screenshot from 2022-03-31 15-50-59
Screenshot from 2022-03-31 19-14-21

Additional references

Documentation updated

@janelletavares janelletavares requested review from a team, ahmeroxa and anna-cross and removed request for a team April 1, 2022 02:18
janelletavares added 2 commits March 31, 2022 19:50
Copy link
Member

@raulb raulb left a comment

Choose a reason for hiding this comment

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

Yesterday I was chatting with @jayjayjpg about this one, and I was wondering if this information could be returned by the API directly, so we wouldn't need to fetch for each of the resources. Possibly having even two different endpoints such as:

  • GET /v1/applications/:uuid (no --extended)
  • GET /v1/applications/:uuid?extended (with --extended).

What do you think?

P.S..: This question is not a blocker just an alternative implementation in case the list of connectors / functions / etc... ends up being too big.

@janelletavares
Copy link
Contributor Author

Yesterday I was chatting with @jayjayjpg about this one, and I was wondering if this information could be returned by the API directly, so we wouldn't need to fetch for each of the resources. Possibly having even two different endpoints such as:

  • GET /v1/applications/:uuid (no --extended)
  • GET /v1/applications/:uuid?extended (with --extended).

What do you think?

P.S..: This question is not a blocker just an alternative implementation in case the list of connectors / functions / etc... ends up being too big.

the current implementation is quite slow. doing it all in the API would be faster and nicer. i could go either way, but at this point, i would favor making time for python work over another implementation of this; i'd be happy to open a follow up ticket to improve performance/efficiency around this. how does that sound?

@jayjayjpg
Copy link
Contributor

@janelletavares Yes, fully agree with you and that sounds good to me!

@janelletavares
Copy link
Contributor Author

opened this issue https://github.com/meroxa/turbine-project/issues/129

return output
}

func extendedResourcesTable(resources []*meroxa.Resource, connectors map[string]*meroxa.Connector) string {
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'm still not sure why, but the typical library used for these tables was giving me lots of issues with nil pointers when trying to format these two subtables, so i just went with a less flexible option

@janelletavares janelletavares merged commit 71e537a into master Apr 2, 2022
@janelletavares janelletavares deleted the extended-app-describe branch April 2, 2022 00:32
@raulb
Copy link
Member

raulb commented Apr 4, 2022

the current implementation is quite slow. doing it all in the API would be faster and nicer. i could go either way, but at this point, i would favor making time for python work over another implementation of this; i'd be happy to open a follow up ticket to improve performance/efficiency around this. how does that sound?

@janelletavares makes sense to me

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.

3 participants