Skip to content

Conversation

@fizzboop
Copy link
Contributor

@fizzboop fizzboop commented Apr 16, 2025

Since we have moved away from using .toml files in extensions, we should remove this one in the Reaper. This was also causing a flaky error in the connect_gallery/home.cy.js test because the .toml file was included in the bundle in the PR from removing references to .toml files

@fizzboop fizzboop self-assigned this Apr 16, 2025
@cgraham-rs
Copy link
Contributor

Is it important right now to delete every instance in the repo to prevent potential gallery problems?

❯ find . -name "connect-extension.toml"
./extensions/notifier/connect-extension.toml
./extensions/datadog-prometheus-metrics/connect-extension.toml
./extensions/oauth-integration-validator/connect-extension.toml
./extensions/audit-api/connect-extension.toml
./extensions/integration-session-manager/connect-extension.toml
./extensions/prototype-content-with-issues-table/connect-extension.toml
./extensions/reaper/connect-extension.toml
./extensions/who-deploys-most-often/connect-extension.toml
./extensions/publisher-command-center/connect-extension.toml
./extensions/audit-reports/connect-extension.toml
./extensions/oauth-integration-debug/connect-extension.toml

Copy link
Contributor

@cgraham-rs cgraham-rs left a comment

Choose a reason for hiding this comment

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

We are going to have to also edit the version in the manifest or it won't release the changes.

@jonkeane
Copy link
Collaborator

I'm 💯 for 🚮 these (now old) files, but:

This was also causing a flaky error in the connect_gallery/home.cy.js test because the .toml file was included in the bundle in the https://github.com/posit-dev/connect/pull/30998 from removing references to .toml files

If someone includes an extension.toml file, that should be ignored during the deployment process with the new PR, right? I'm surprised there was a test failure/flake related to inclusion of a now-ignored file. Unless we were testing for the absence of this file or something?

@fizzboop
Copy link
Contributor Author

If someone includes an extension.toml file, that should be ignored during the deployment process with the new PR, right? I'm surprised there was a test failure/flake related to inclusion of a now-ignored file. Unless we were testing for the absence of this file or something?

@jonkeane: I think we are because I tested three links of bundles without .toml files (portfolio-dashboard, quarto-document, and quarto-stock-report-python) and they passed E2E locally. I'm not sure where exactly we are testing for the lack of a .toml, but I would like to learn how that's happening.

@dotNomad
Copy link
Collaborator

Is it important right now to delete every instance in the repo to prevent potential gallery problems?

It isn't, but would be a great cleanup to avoid confusion. I left them as not all content has a new manifest.extension section and the content of the connect-extension.toml file will be helpful to fill that since the descriptions are already written.

@dotNomad
Copy link
Collaborator

This was also causing a flaky error in the connect_gallery/home.cy.js test because the .toml file was included in the bundle in the https://github.com/posit-dev/connect/pull/30998 from removing references to .toml files

@jonkeane: I think we are because I tested three links of bundles without .toml files (portfolio-dashboard, quarto-document, and quarto-stock-report-python) and they passed E2E locally. I'm not sure where exactly we are testing for the lack of a .toml, but I would like to learn how that's happening.

Did a bit of pairing on this, and we believe that the flakiness was due to hitting the Cypress timeout waiting for the app to fully deploy. The restoring of the Python environment can take a bit of time, so that would explain the flake on the test.

@fizzboop fizzboop merged commit 690a9b9 into main Apr 16, 2025
12 checks passed
@fizzboop fizzboop deleted the bh-remove-reaper-toml branch April 16, 2025 22:29
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.

5 participants