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

Remove frontend code #35

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Remove frontend code #35

merged 1 commit into from
Jan 11, 2022

Conversation

andresmgot
Copy link
Contributor

Frontend code is now at https://github.com/grafana/grafana-aws-sdk-react

Closes #26
Closes #27 (CI/CD is already set in both repos)

Copy link
Collaborator

@sunker sunker left a comment

Choose a reason for hiding this comment

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

I'm not sure it's a good idea to remove this before we have the new repo fully functioning and (at least one) plugin consumes it. Maybe we can keep this PR open until then?

@andresmgot
Copy link
Contributor Author

andresmgot commented Nov 5, 2021

I think it's safe. In the unlikely event that we need an urgent update of the NPM package and the other repo is somehow broken we could always revert this commit but I guess it doesn't hurt to be extra careful.

@sarahzinger
Copy link
Member

It's probably safe to remove but I think it'll be confusing if anyone new joins and is trying to understand where this code is coming from, +1 for keeping this pr open until we have an alternative ready to be consumed, but otherwise this is a ✅ from me!

@ryantxu
Copy link
Member

ryantxu commented Nov 8, 2021

Any insights on the motivation for this? is it just to have different repos for frontend+backend. Is there anything wrong with having them together?

obviously if we have two repos, the same code should not be duplicated in both places!

@andresmgot
Copy link
Contributor Author

Any insights on the motivation for this? is it just to have different repos for frontend+backend. Is there anything wrong with having them together?

The major reason is that the two code bases are not related. Apart from that:

  • It's easier/faster to set CI (you only run go or typescript tests)
  • To make releases are consistent. If you go to the releases of this repo: https://github.com/grafana/grafana-aws-sdk/tags, those are go versions. In order to know what is the latest version of the NPM package, you need to go to the package.json or to the package view that by the way, they report different versions.

@andresmgot andresmgot mentioned this pull request Nov 9, 2021
@sunker
Copy link
Collaborator

sunker commented Nov 9, 2021

Although not technically bound together, I guess they are related in the sense that they both interact with the data source json model. However, keeping them in one repo does not cater for safe, canonical interaction with the json model.

@ryantxu are there any reasons for keeping them in the same repo? The only one I can think of is if we could use a shared schema for describing the data source jsondata model. That way we could theoretically build the backend and the frontend together. You know this better so correct me if I'm wrong, but I believe that's not something that the cue will be able to solve anytime soon?

Here are my reasons for splitting the repo. None of them are absolutely necessary, but I think it's the combination of them that makes it the right decision:

  • It makes for cleaner code
  • Consistent releases, having each repo produce one release/artifact only
  • Should make CI slightly easier
  • Makes local development slightly easier, not having to yarn link the go packages that you don't need in your frontend code
  • Following the same structure as the google and the azure sdk, which should make life easier for our team since we're the ones maintaining all these sdk:s

Today, the ui components (ConnectionConfig.tsx for example) are very much related to the go packages in the SDK. However, in the next couple of weeks we'll be making a whole bunch of additions to the SDK. See this epic. For these shared components/packages, there's no link at all between frontend and backend.

@ryantxu
Copy link
Member

ryantxu commented Nov 9, 2021

@sunker -- only reason is to avoid having lots of repos, and no real good reason to keep them separate. github has gotten better at managing cross-repo and that cat is a bit out of the bag so 🤷‍♂️

I don't feel too strongly, just don't see any reason for it. In parallel we trying to figure out how to have multiple projects/plugins in a single repo.

@andresmgot
Copy link
Contributor Author

andresmgot commented Jan 10, 2022

Released several versions of the package in the new repository so it's safe to remove it from here:

Version Downloads (Last 7 Days) Published
0.0.32 0 an hour ago
0.0.32-pre.3 27 20 days ago
0.0.32-pre.2 3 25 days ago
0.0.32-pre.1 4 25 days ago

https://www.npmjs.com/package/@grafana/aws-sdk

@sunker
Copy link
Collaborator

sunker commented Jan 11, 2022

Seems like this PR is in the wrong repo.

Copy link
Collaborator

@sunker sunker left a comment

Choose a reason for hiding this comment

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

LGTM

@andresmgot andresmgot merged commit 4d664a7 into main Jan 11, 2022
@andresmgot andresmgot deleted the removeFrontend branch January 11, 2022 08:45
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.

Setup CI/CD for this repo Split the repo in two
4 participants