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

Isolate react-app package #3589

Merged
merged 21 commits into from
Nov 3, 2023
Merged

Conversation

alexweav
Copy link
Contributor

@alexweav alexweav commented Nov 1, 2023

This PR changes the package dependency hierarchy such that ui no longer depends on ui/react-app:

What was once this:

└── cmd/alertmanager/
    └── ui/
        ├── app
        └── react-app

Now becomes:

└── cmd/alertmanager/
    ├── ui/
    │   └── app
    └── react-app

i.e. the react-app endpoints are now registered separately.

This has a few benefits:

  • Isolates the problem described in github.com/prometheus/alertmanager/ui package cannot be vendored #3588. reactApp has custom build tooling, downstream projects of alertmanager can choose to independently omit this if they don't wish to run that toolchain to compile.
  • Due to Go's package tree-shaking, downstream projects can selectively choose which UI they want to include (or both, or neither). This makes the alertmanager dependency lighter.
  • Downstream projects like Mimir or Thanos which don't want to serve the react app quite yet can choose to register the routes independently.

Signed-off-by: Alex Weaver <[email protected]>
@@ -0,0 +1,51 @@
package reactApp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All code in this file is purely extracted from the old ui/web.go. No substantive changes.

Copy link
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM - this seems sensible.

// See the License for the specific language governing permissions and
// limitations under the License.

package reactApp
Copy link
Member

Choose a reason for hiding this comment

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

For the sake of simplcity, let's call this package reactUI and name the file accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

nvm, as you explained offline there's benefits to this approach - downstream consumers don't have to adopt the react-ui until it's ready. I'm good with that.

@gotjosh
Copy link
Member

gotjosh commented Nov 1, 2023

There's one more thing I think we should do here - let's make sure we version control the react-generated, and we also need a CI step that aborts if there's a difference between what we have and what is generated.

Otherwise, we don't have visibility when changes are made.

@alexweav alexweav force-pushed the alexweav/isolate-react-app branch from 551188e to 3adf7b4 Compare November 1, 2023 20:05
ui/react-app/tsconfig.json Outdated Show resolved Hide resolved
@alexweav alexweav force-pushed the alexweav/isolate-react-app branch 2 times, most recently from 4e62c4c to 57fedd3 Compare November 1, 2023 21:21
@alexweav alexweav force-pushed the alexweav/isolate-react-app branch from 72bcb9f to a5339f5 Compare November 2, 2023 00:51
@alexweav alexweav force-pushed the alexweav/isolate-react-app branch from 3168c5d to 7c53ef7 Compare November 2, 2023 19:06
@alexweav
Copy link
Contributor Author

alexweav commented Nov 2, 2023

This test has flaked out more than once: issue filed. #3593

Another was CircleCI failing to talk to NPM to download a dependency. This one seemed like a one-off.

Signed-off-by: Alex Weaver <[email protected]>
@@ -334,7 +334,7 @@ func testTLSConnection(t *testing.T) {
)
require.NoError(t, err)
go p2.Settle(context.Background(), 0*time.Second)

p2.WaitReady(context.Background())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drive-by fix; this test flaked a couple times. #3593

The go Settle and following assert race each other. Other tests, including further up in this same test, use a WaitReady - it seems to be missing here.

@@ -7,9 +7,9 @@ set -euo pipefail
cd ui/react-app
cp embed.go.tmpl embed.go

GZIP_OPTS="-fk"
GZIP_OPTS="-fkn"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

-n means to avoid including file naming information.

This also happens to exclude file timestamp info, which keeps the built artifacts reproducible. Otherwise, you get diff in the compressed files every time they're rebuilt.

@@ -43,3 +43,4 @@ jobs:
- uses: prometheus/[email protected]
- uses: ./.github/promci/actions/setup_environment
- run: make
- run: git diff --exit-code
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This step will fail if the artifacts are not updated. The resulting behavior is the same as the legacy UI.

Tested as part of this PR, the test is visible in this PR's commit history.

Copy link
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

Beautiful! Thanks very much for your contribution.

# gzip option '-k' may not always exist in the latest gzip available on different distros.
if ! gzip -k -h &>/dev/null; then GZIP_OPTS="-f"; fi
if ! gzip -k -h &>/dev/null; then GZIP_OPTS="-fn"; fi
Copy link
Member

Choose a reason for hiding this comment

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

My only worry is that browsers might use these headers to know whether they should cache the bundle, but a quick Google search yielded no results.

I'll let @Nexucis and @juliusv advise on whether this is used but I'll merge for now because overall it's a nest positive for upstream consumers.

@gotjosh gotjosh merged commit fdea7e7 into prometheus:main Nov 3, 2023
7 checks passed
@juliusv
Copy link
Member

juliusv commented Nov 3, 2023

I don't mind the UI package not dragging along more dependencies than needed, but checking in the entire build output for the React app seems a bit unusual to me - it's almost like checking in a compiled binary, and now you'll have commit it again for every little change in the source. In Prometheus we circumvent this problem in a different way: the embed.go.tmpl file contains a Go build tag that only builds this file when the builtinassets Go build flag is set. This flag is set when building with promu (which we use for releases), but when someone just uses the package as a Go dependency and the flag is not set, it just pulls in an empty UI via https://github.com/prometheus/prometheus/blob/main/web/ui/ui.go (which is only built when that build tag is not set). I'd prefer if we could keep Prometheus and Alertmanager in sync in that respect.

grobinson-grafana pushed a commit to grobinson-grafana/alertmanager that referenced this pull request Nov 9, 2023
* Isolate react-app package

Signed-off-by: Alex Weaver <[email protected]>

---------

Signed-off-by: Alex Weaver <[email protected]>
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