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

absorb new meroxa-go interface; small doc changes #217

Merged
merged 18 commits into from
Nov 19, 2021
Merged

Conversation

janelletavares
Copy link
Contributor

Description of change

absorb new meroxa-go interface; small doc changes

Fixes

Type of change

  • New feature
  • Bug fix
  • Refactor
  • Documentation

How was this tested?

  • Unit Tests
  • Tested in staging

Demo

Before this pull-request

Include how it looked before

After this pull-request

Include how it looks now

Additional references

Any additional links (if appropriate)

Documentation updated

Make sure that our documentation is accordingly updated when necessary.

You can do that by opening a pull-request to our (🔒 private, for now) repository: https://github.com/meroxa/meroxa-docs.

✨ In the future, there will be a GitHub action taking care of these updates automatically. ✨

Provide PR link:

@janelletavares janelletavares requested review from a team, owenthereal and simonl2002 and removed request for a team November 4, 2021 19:15
Copy link
Contributor

@dianadoherty dianadoherty left a comment

Choose a reason for hiding this comment

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

In a couple of places I see reference to NameOrID as just name, it might be worth it to clarify that it can be either

cmd/meroxa/root/connectors/describe.go Show resolved Hide resolved
cmd/meroxa/root/connectors/logs.go Show resolved Hide resolved
cmd/meroxa/root/connectors/remove.go Show resolved Hide resolved
cmd/meroxa/root/connectors/remove.go Show resolved Hide resolved
@raulb
Copy link
Member

raulb commented Nov 10, 2021

@janelletavares started reviewing this pull-request, but I think it lacks some context on the motivation behind it. Maybe adding a link to the change made in meroxa-go would be useful. I appreciate we're moving away from having the CLI to define their own mocks, but I noticed some other UX changes (messaging, etc) that would be great to detail.

Let me know if I can help with this.

@janelletavares
Copy link
Contributor Author

@janelletavares started reviewing this pull-request, but I think it lacks some context on the motivation behind it. Maybe adding a link to the change made in meroxa-go would be useful. I appreciate we're moving away from having the CLI to define their own mocks, but I noticed some other UX changes (messaging, etc) that would be great to detail.

Let me know if I can help with this.

Here are the related PRs, if that helps.
https://github.com/meroxa/meroxa-go/pull/77
https://github.com/meroxa/meroxa-go/pull/78
https://github.com/meroxa/meroxa-go/pull/79

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.

Thank you @janelletavares for providing those links with changes made in meroxa-go. I still have some questions to what seem to be changing publicly documented flags (metadata, and some expected behavior when creating connectors via meroxa connect).

Let's pair if you think I'm still missing some context.

cmd/meroxa/global/logger.go Show resolved Hide resolved
cmd/meroxa/global/client.go Show resolved Hide resolved
cmd/meroxa/root/connectors/connect_test.go Show resolved Hide resolved
},
Metadata: map[string]interface{}{},
Copy link
Member

Choose a reason for hiding this comment

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

How come we're no longer comparing on Metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value we put in isn't the totality of the value we get out, so it got messy. there may be a way to adjust for that, i didn't spend a lot of time on it.

cmd/meroxa/root/connectors/connect_test.go Outdated Show resolved Hide resolved
cmd/meroxa/root/connectors/create.go Outdated Show resolved Hide resolved
cmd/meroxa/root/connectors/remove.go Show resolved Hide resolved
cmd/meroxa/root/connectors/remove_test.go Show resolved Hide resolved
cmd/meroxa/root/pipelines/create.go Outdated Show resolved Hide resolved
cmd/meroxa/root/pipelines/update.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@janelletavares janelletavares left a comment

Choose a reason for hiding this comment

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

thanks for the review!

cmd/meroxa/global/client.go Show resolved Hide resolved
cmd/meroxa/global/logger.go Show resolved Hide resolved
cmd/meroxa/root/connectors/connect_test.go Show resolved Hide resolved
},
Metadata: map[string]interface{}{},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value we put in isn't the totality of the value we get out, so it got messy. there may be a way to adjust for that, i didn't spend a lot of time on it.

cmd/meroxa/root/connectors/connect_test.go Outdated Show resolved Hide resolved
cmd/meroxa/root/connectors/create.go Outdated Show resolved Hide resolved
cmd/meroxa/root/connectors/remove.go Show resolved Hide resolved
cmd/meroxa/root/pipelines/create.go Outdated Show resolved Hide resolved
cmd/meroxa/root/pipelines/update.go Outdated Show resolved Hide resolved
@raulb
Copy link
Member

raulb commented Nov 17, 2021

@janelletavares Working on the CLI today I realized I'm blocked on some changes made in meroxa-go, so I'll need make some of these changes myself to unblock environments' work, unless this pull-request is broken out in different parts. My biggest concern on this pull-request are the user breaking changes that would need to be communicated to customers (removing support on feature flags).

If we could have only one pull-request that could make the CLI work with the latest of meroxa-go would be great, thank you.

@janelletavares janelletavares requested a review from raulb November 18, 2021 18:47
@janelletavares
Copy link
Contributor Author

@raulb if the meroxa-go PR and this PR seem good to you, feel free to squash merge, update the go.mod to unblock you.

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.

This LGTM now. We'll evaluate removing the metadata flag in a separate pull-request following the usual process:

  1. Check usage of this flag to determine a deprecation path (or simply removing it if it hasn't been used)
  2. Depending on 1, hide flag, set a deprecation date to remove it altogether
  3. Remove support in meroxa-go

@raulb raulb merged commit 725bf81 into master Nov 19, 2021
@raulb raulb deleted the janelle_docs branch November 19, 2021 11:35
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