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

Add test case for mysqldump deprecation #54

Merged
merged 7 commits into from
Jun 11, 2024

Conversation

christopher-b
Copy link
Collaborator

This add a test case for a proposed change to lando/core to address #47.

Bare minimum self-checks

What do you think of a person who only does the bare minimum?

  • I've updated this PR with the latest code from main
  • I've done a cursory QA pass of my code locally
  • I've ensured all automated status check and tests pass
  • I've connected this PR to an issue

Pieces of flare

  • I've written a unit or functional test for my code
  • I've updated relevant documentation it my code changes it
  • I've updated this repo's README if my code changes it
  • I've updated this repo's CHANGELOG with my change unless its a trivial change (like updating a typo in the docs)

Finally

If you have any issues or need help please join the #contributors channel in the Lando slack and someone will gladly help you out!

You can also check out the coder guide.

This is a test for a proposed change to lando/core, to address
lando#47.
Copy link

netlify bot commented Apr 17, 2024

Deploy Preview for lando-mariadb ready!

Name Link
🔨 Latest commit 162a259
🔍 Latest deploy log https://app.netlify.com/sites/lando-mariadb/deploys/6668d57e55d92c00080d9baf
😎 Deploy Preview https://deploy-preview-54--lando-mariadb.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 92 (🔴 down 1 from production)
Accessibility: 98 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Make better use of `grep` in the test, and direct output of dump command
to STDOUT so the test doesn't generate export files.
@reynoldsalec
Copy link
Member

@christopher-b the one thing I'd be curious about verifying is whether recipes that inherit MariaDB will use the script you've included in the MariaDB plugin's scripts folder, or if they'll use the version in Lando Core: https://github.com/lando/core/blob/main/scripts/sql-export.sh

I'm not sure which will take precedence. To test, I'd download a recipe, and after you run npm install on it, replace the node_modules/@lando/mariadb dependency with your version of MariaDB, then use one of the recipe's examples directory apps to see what happens.

@christopher-b
Copy link
Collaborator Author

Hey @reynoldsalec! Since the script in this recipe is in example/tooling/scripts, I wouldn't imagine it would get auto-included anywhere, yeah?

I realized I didn't reference the other PR (in lando/core) here, which is probably contributing to some confusion.

Copy link
Contributor

@rubenvarela rubenvarela left a comment

Choose a reason for hiding this comment

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

Looks good

@reynoldsalec
Copy link
Member

@christopher-b thinking about it, I'm wondering if the tooling command you're adding should simply copy what the recipes (LAMP/Drupal/Laravel/etc.) use for their db-export commands and place that in one of the existing example folder's .lando.yml: https://github.com/lando/lamp/blob/main/builders/lamp.js#L38

Basically we're trying to make sure the script added in core is working. The tooling commands are distributed across repos, so testing here is a centralized proxy for testing on ALL recipes that implement a db-export tooling command.

Copy link

@xurizaemon xurizaemon left a comment

Choose a reason for hiding this comment

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

One tiny typo in introduced code.

Does the command and script name pattern follow the MySQL implementation? I noticed the command is db-export and the script is scripts/sql-export.sh, just checking that's not a new variation. I don't see analogous export script in https://github.com/lando/mysql/tree/main - looks like that's in landó/core only?

Edit: yeah that change in lando/core#148 is more what I expected to see than adding a new sql-export.sh script here.

examples/tooling/scripts/sql-export.sh Outdated Show resolved Hide resolved
@christopher-b
Copy link
Collaborator Author

I thinking that once lando/core#148 is merged, we should nix this PR and introduce a new one with the test that @reynoldsalec is suggesting.

@reynoldsalec
Copy link
Member

I merged the PR for lando/core, but I think we'll need to wait until a release is cut for lando/core to test this out; I'm going to check and see if there's anything else we should get into core for a release, so might delay progress here until early next week.

Copy link
Collaborator

@uberhacker uberhacker left a comment

Choose a reason for hiding this comment

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

See comment.

@reynoldsalec reynoldsalec merged commit e1ee8c2 into lando:main Jun 11, 2024
26 checks passed
@reynoldsalec reynoldsalec mentioned this pull request Aug 6, 2024
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