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

feat: Run Core Script, Script Options, Script Consolidation #9

Merged
merged 8 commits into from
Apr 24, 2022

Conversation

JeremyEastham
Copy link
Contributor

  • Adds a new script called runCore that starts the testing environment and runs the core
  • Adds options to startTestingEnv and runCore
    • -h or --help displays usage info
    • -w or --wait uses existing behavior (startTestingEnv only)
    • -s or --silent suppresses echoes in both scripts
    • -c or --cicd tells the script to use the CI/CD versions of setupTestEnv and cleanTestEnv
    • -f or --force tells the script to destroy the testing environment if a .testEnvStarted file exists
      • This option is useful if you terminate a script early and the cleanup routine does not run, leaving the environment behind

- Observe visibility of `.testEnvStarted` (`--force` to remove previous environment)
- Add `--help` option, `--silent` option, and `--force` option
- Add argument aliases (`-w` for `--wait`, etc)
- Add "Run Core" run configuration for `./runCore`
- Added options `--help`, `--silent`, `--cicd`, and `--force` with aliases
- Handles both Linux/Mac and Windows paths using cygpath
@rishabhpoddar
Copy link
Member

@JeremyEastham please lmk when this is ready for review :) You can assign me as a reviewer.

- `startTestingEnv` is now `startTestEnv`
- `setupTestEnvLocal` is now `setupTestEnv --local` or `setupTestEnv`
- `cleanTestEnvLocal` is now `cleanTestEnv --local` or `cleanTestEnv`
- `setupTestEnvCicd` is now `setupTestEnv --cicd`
- `cleanTestEnvCicd` is now `cleanTestEnv --cicd`
@JeremyEastham JeremyEastham marked this pull request as draft April 22, 2022 15:27
@JeremyEastham
Copy link
Contributor Author

I have consolidated setupTestEnv, cleanTestEnv, and startTestEnv.

  • startTestingEnv is now startTestEnv
  • setupTestEnvLocal is now setupTestEnv --local or setupTestEnv
  • cleanTestEnvLocal is now cleanTestEnv --local or cleanTestEnv
  • setupTestEnvCicd is now setupTestEnv --cicd
  • cleanTestEnvCicd is now cleanTestEnv --cicd

These changes not only make the commands clearer (since most of the time you want to run the local version anyway), but they also make the scripts much DRYer. We now only have one script to edit for each part of the development process.

I have made sure to replace all occurrences of startTestingEnv with startTestEnv in all strings, READMEs, comments, and code. I don't believe that script is mentioned on the docs website, since it is only used for developing the core, but I can look if you think it might be in there somewhere.

I have converted this PR to a draft because I need to submit a PR for the core with the naming changes so it doesn't break the automatic tests.

@rishabhpoddar
Copy link
Member

These changes seem great!! Really cool!

There is just one big problem though -> All the other repos use ./utils/setupTestEnvLocal to setup the dev env during CICD tests. For example:

And the above are just some of the examples. I think a good solution to this would be to keep the older scripts where they are and make those call the new scripts. We can add comments in the older ones saying why they exist and to not use them, or we can add an echo in them saying to use the new scripts instead. Then over time, we can change the other repos to use the newer scripts and then finally get rid of the older scripts.

What do you think?

@JeremyEastham
Copy link
Contributor Author

I am preparing a PR that updates the GitHub Actions and CircleCI CI/CD scripts. I will make the existing scripts echo a deprecation warning to stderr so that any scripts that I miss can be updated. These deprecated scripts can call the correct scripts with the default options so that existing tasks will still work.

@JeremyEastham JeremyEastham changed the title feat: Run Core Script and Script Options feat: Run Core Script, Script Options, Script Consolidation Apr 23, 2022
@JeremyEastham
Copy link
Contributor Author

After all of this is done, I eventually plan to migrate the functionality of these scripts into either Gradle Tasks, or maybe a separate development-only Java module. This would eliminate the need for platform-specific code or development setup.

Currently, the runBuild scripts in each repo are very slow due to needing to build each jar from scratch every time. If we migrate to Gradle, incremental compilation will allow the compiler to skip recompiling the modules that have not changed, substantially decreasing build and run time during development.

@JeremyEastham
Copy link
Contributor Author

I have implemented the changes that you suggested. If one of the deprecated scripts is called, it now prints a deprecation warning in red to stderr. This PR can be merged if it looks good to go.

@JeremyEastham JeremyEastham marked this pull request as ready for review April 23, 2022 08:27
@JeremyEastham
Copy link
Contributor Author

I just changed ./utils/pre-commit thinking that this was the pre-commit hook that is run in supertokens-core. I updated it and it did not update the hook. I looked deeper and discovered that the actual hook is located in supertokens-root/supertokens-core/.git/pre-commit, which looks to be a similar or identical script to supertokens-root/utils/pre-commit. Is the script in utils used anywhere? If not, I will delete it so it doesn't confuse us.

@JeremyEastham
Copy link
Contributor Author

JeremyEastham commented Apr 23, 2022

How can I commit my changes to the hook for the core? Even if I run git add .git/hooks/pre-commit, it does not show up in git status or git diff.

It does run the updated script when I commit, but I'm not sure if it will update the hook for everyone else.

@rishabhpoddar
Copy link
Member

I just changed ./utils/pre-commit thinking that this was the pre-commit hook that is run in supertokens-core. I updated it and it did not update the hook. I looked deeper and discovered that the actual hook is located in supertokens-root/supertokens-core/.git/pre-commit, which looks to be a similar or identical script to supertokens-root/utils/pre-commit. Is the script in utils used anywhere? If not, I will delete it so it doesn't confuse us.

When you run ./loadModules, it copies the pre-commit hook from ./utils/pre-commit into all the sub projects in the right place. So changing ./utils/pre-commit should be good enough.

@rishabhpoddar
Copy link
Member

I am preparing a PR that updates the GitHub Actions and CircleCI CI/CD scripts.

If you are doing this for all the other repos, here is a list of them:

  • supertokens-core
  • supertokens-postgresql-plugin
  • supertokens-mysql-plugin
  • supertokens-mongodb-plugin
  • supertokens-sql-plugin (only needs an update for the GH action)
  • supertokens-node
  • supertokens-golang
  • supertokens-python
  • supertokens-website
  • supertokens-react-native
  • supertokens-auth-react

@rishabhpoddar
Copy link
Member

Is this ready for review and merging?

@JeremyEastham
Copy link
Contributor Author

Yes, it is. Please double-check the deprecated scripts so that this PR doesn't interrupt any CI/CD pipelines.

@rishabhpoddar rishabhpoddar merged commit d7b0cf1 into supertokens:master Apr 24, 2022
@rishabhpoddar
Copy link
Member

@JeremyEastham i noticed a few issues:

  • The new scripts were not executable. I changed their permission and pushed them to git
  • The runCore script starts to build in the background. But if a kill signal is given to it, it doesn't stop the background building process (while it's building).

@JeremyEastham
Copy link
Contributor Author

Thanks for explaining these issues. I'm surprised that Git Bash ran the scripts fine, since they weren't executable. Did you fix the kill signal issue?

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.

None yet

2 participants