Upgrade vagrant environment #492
Conversation
| @@ -6,8 +6,9 @@ PGUSER=atr | |||
| PGPASSWORD=atr | |||
| PGHOST=localhost | |||
| PGPORT=5432 | |||
| ENVIRONMENT=dev | |||
There was a problem hiding this comment.
@evmiguel I see dev being referenced in embed.js but I'm assuming development is being inferred throughout when this ENV var doesn't exist. Should those embed.js references to this var (or lack of) also be updated?
There was a problem hiding this comment.
@howard-e Good catch! Since the vagrant environment mirrors production, it's technically not using the dev configuration. I think we should name the variable something else with a different value.
There was a problem hiding this comment.
What would be the consequence of leaving that variable unchanged?
There was a problem hiding this comment.
@jugglinmike The app would look for the server side rendering files for the embed feature in a different directory.
deploy/roles/nodejs/tasks/main.yml
Outdated
| @@ -8,6 +8,32 @@ | |||
| - https://dl.yarnpkg.com/debian/pubkey.gpg | |||
| - https://deb.nodesource.com/gpgkey/nodesource.gpg.key | |||
|
|
|||
| - name: Check if Node exists | |||
There was a problem hiding this comment.
This is the part that automatically removes any Node version that is not version 18.
There was a problem hiding this comment.
Node isn't installed in the "debian/buster64" Vagrant box.
There was a problem hiding this comment.
I should have been clearer here. Lines 11-45 take care of the Node upgrade.
There was a problem hiding this comment.
Ah, so this is not about the Vagrant environment. Is it right to say that these new tasks are intended to support transitioning the production, staging, and "sandbox" environments?
There was a problem hiding this comment.
Yes, that's right!
There was a problem hiding this comment.
Got it! How about placing instructions in a dedicated file and only including it for production? That will more accurately reflect the intent, optimize the "nodejs" role, and make it easier to safely remove the transitional tasks once all the running systems have been updated.
There was a problem hiding this comment.
@jugglinmike I placed the tasks here.
deploy/roles/nodejs/tasks/main.yml
Outdated
| - name: Remove Node.js repo if not version 18 | ||
| apt_repository: | ||
| state: absent | ||
| repo: deb https://deb.nodesource.com/node_14.x buster main |
evmiguel
force-pushed
the
upgrade-vagrant-environment
branch
from
d7ffc84 to
70d370e
Compare
There was a problem hiding this comment.
Yarn is refusing to install the dependencies in Node.js version 18.
$ yarn install
yarn install v1.22.19
[1/4] Resolving packages...
[2/4] Fetching packages...
error apicache@1.6.2: The engine "node" is incompatible with this module. Expected version ">=8 <=15". Got "18.14.0"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
| args: | ||
| chdir: '{{source_dir}}' | ||
|
|
||
| - name: Seed database | ||
| shell: DOTENV_CONFIG_PATH={{environment_config.dest}} node -r ./node_modules/dotenv/config $(npm bin)/sequelize-cli db:seed:all --config ./config/config.js --seeders-path ./server/seeders/ | ||
| shell: DOTENV_CONFIG_PATH={{environment_config.dest}} node -r ./node_modules/dotenv/config ./node_modules/.bin/sequelize-cli db:seed:all --config ./config/config.js --seeders-path ./server/seeders/ |
There was a problem hiding this comment.
Note to other reviewers: npm recently removed this subcommand.
| register: new_node_version | ||
|
|
||
| - name: Show status of new nodejs installation | ||
| debug: msg="{{ new_node_version.stdout }}" |
There was a problem hiding this comment.
It's safe to trust apt to tell us if the installation failed.
There was a problem hiding this comment.
I wanted this debug message for us to compare the version in the Check if Node exists task and the node version installed during the upgrade.
deploy/roles/nodejs/tasks/main.yml
Outdated
| @@ -8,6 +8,32 @@ | |||
| - https://dl.yarnpkg.com/debian/pubkey.gpg | |||
| - https://deb.nodesource.com/gpgkey/nodesource.gpg.key | |||
|
|
|||
| - name: Check if Node exists | |||
There was a problem hiding this comment.
Node isn't installed in the "debian/buster64" Vagrant box.
| when: deployment_mode != 'development' | ||
|
|
||
| # The job is written to change the permissions of the /vagrant/server | ||
| # directory because the POST request does not have permission to it |
There was a problem hiding this comment.
I'm confused about this. Since the curl utility is issuing an HTTP request to the web server, it doesn't seem like it needs access to any particular directories on the file system.
There was a problem hiding this comment.
@jugglinmike nice catch! I should have worded this differently. The process that is invoked through the POST request accesses the /vagrant/server folder.
There was a problem hiding this comment.
I see that you've removed the mode changes from the command, but the comment still describes the earlier version.
evmiguel
requested review from
jugglinmike and
howard-e
and removed request for
jugglinmike
|
@jugglinmike did you have a vagrant machine already up when you ran the vagrant deployment in this branch? |
I did not. That said, I'm not able to reproduce the error during |
|
@howard-e @jugglinmike there are new instructions in the description to simulate an upgrade with Vagrant. Let me know your thoughts and if you feel confident in trying them out on the other environments afterwards. |
|
Following the procedure in this pull request's description, I observed Followed later by So that aspect appears to be working as intended! |
evmiguel
force-pushed
the
upgrade-vagrant-environment
branch
from
fc9f758 to
6bfdf7e
Compare
evmiguel
changed the title
There was a problem hiding this comment.
LGTM especially having also tested this with a deployment to the sandbox environment as well, but I'd leave the final thoughts to @jugglinmike on this as there are still unresolved discussions.
|
I'm still learning about the environments this application supports, but I'm wondering if whether the introduction of a variable named The following table is how I'm interpreting of the state of affairs represented by this patch. The first two columns describe the potential values of the environment variables we're discussing; the next three columns describe the effect each combination has on the application, and the final column defines the system where the combination is actually used.
It seems to me that introducing an independent variable which controls only the embed link protocol creates space for many situations that this application doesn't truly need--see all the "(unused)" rows. If we could fix support for Vagrant just by introducing a new value for Here's what I have in mind:
-const protocol = process.env.ENVIRONMENT === 'dev' ? 'http://' : 'https://';
+const protocol =
+ process.env.DEPLOY_ENVIRONMENT === 'dev' ? 'http://' : 'https://';This patch could implement the following change: -const protocol = process.env.ENVIRONMENT === 'dev' ? 'http://' : 'https://';
+const protocol =
+ /dev|vagrant/.test(process.env.ENVIRONMENT) ? 'http://' : 'https://';If I haven't overlooked anything in this design (and that's a big "if"), this would improve the project's onboarding experience since new contributors wouldn't have to familiarize themselves with subtle distinctions in the combination of like-named variables. (This interpretation suggests an opportunity for further simplifications, such as using What do you think, @evmiguel? |
evmiguel
force-pushed
the
upgrade-vagrant-environment
branch
from
65d324d to
f303d96
Compare
|
@jugglinmike Thank you for your thoughtful analysis and consideration of the onboarding process with respect to the design of the environment variables. I agree, and I've added your implementation to |
There was a problem hiding this comment.
@jugglinmike Thank you for your thoughtful analysis and consideration of the onboarding process with respect to the design of the environment variables. I agree, and I've added your implementation to
embed.js.
Happy to hear that! I think there's just one more modification necessary to make it work--comment inline.
deploy/files/config-development.env
Outdated
| @@ -6,8 +6,10 @@ PGUSER=atr | |||
| PGPASSWORD=atr | |||
| PGHOST=localhost | |||
| PGPORT=5432 | |||
| ENVIRONMENT=dev | |||
| DEPLOY_ENVIRONMENT=dev | |||
There was a problem hiding this comment.
Rather than
-ENVIRONMENT=dev
+DEPLOY_ENVIRONMENT=devI think we need
-ENVIRONMENT=dev
+ENVIRONMENT=vagrantThere was a problem hiding this comment.
@jugglinmike all set!
evmiguel
force-pushed
the
upgrade-vagrant-environment
branch
from
3340aff to
0573e55
Compare
There was a problem hiding this comment.
The Vagrant environment ought to have the ALLOW_FAKE_ROLE environment variable set. I don't have "write" permissions for this repository, so I'm afraid I can't make this change myself.
diff --git a/deploy/files/config-development.env b/deploy/files/config-development.env
index a5f3d428..77472e89 100644
--- a/deploy/files/config-development.env
+++ b/deploy/files/config-development.env
@@ -7,6 +7,7 @@ PGPASSWORD=atr
PGHOST=localhost
PGPORT=5432
ENVIRONMENT=vagrant
+ALLOW_FAKE_ROLE=true
IMPORT_CONFIG=/home/aria-bot/config.env
GITHUB_OAUTH_SERVER=https://github.com
GITHUB_GRAPHQL_SERVER=https://api.github.comAlso, it looks like we still have issues with filesystem access control. The application responds to requests to import tests with HTTP response code 422, and it prints the following to standard error:
Feb 28 22:05:38 buster node[386]: Listening on 5000
Feb 28 22:11:57 buster node[386]: Command failed: ./deploy/scripts/export-and-exec.sh /home/aria-bot/config.env node ./server/scripts/import-tests/index.js
Feb 28 22:11:57 buster node[386]: Error found: Error: EACCES: permission denied, mkdir '/vagrant/server/scripts/import-tests/tmp'
Feb 28 22:11:57 buster node[386]: at Object.mkdirSync (node:fs:1395:3)
Feb 28 22:11:57 buster node[386]: at module.exports.makeDirSync (/vagrant/server/node_modules/fs-extra/lib/mkdirs/make-dir.js:23:13)
Feb 28 22:11:57 buster node[386]: at readRepo (/vagrant/server/scripts/import-tests/index.js:152:9)
Feb 28 22:11:57 buster node[386]: at importTestPlanVersions (/vagrant/server/scripts/import-tests/index.js:52:57)
Feb 28 22:11:57 buster node[386]: at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
|
@jugglinmike I added the Are you provisioning the changes with I also added you to the admin teams for this repo. Hopefully, you have access now! |
evmiguel
force-pushed
the
upgrade-vagrant-environment
branch
from
aed0003 to
3ecef91
Compare
…onments in embed.js
evmiguel
force-pushed
the
upgrade-vagrant-environment
branch
from
3ecef91 to
4f9dfb9
Compare
That'd be a classic mistake, but I didn't make it this time :) The most direct explanation for the error I reported is that in my instance of the VM, the But that just raises further questions! We both used the same provisioning logic, after all, and that provisioning logic explicitly makes the directory world-writable. It's worth noting that the directory isn't world-writable on my host system, either: If I manually set the bit in the VM, my host system remains unaffected: That's because this project uses the "rsync" folder synchronization strategy. It also means we may have to rethink how we solve this directory permissions problem. Check out how the folder loses the bit after reloading (and automatic synchronization via rsync): So I think I got here because I restarted the VM without reprovisioning. I have a few ideas about how to fix this, but first (since this is potentially dependent on the host OS), can you try reproducing the problem by reloading without reprovisioning?
That did the trick--thanks! |
|
@jugglinmike I can confirm that upon restarting the VM, without reprovisioning, I run into the permissions error. |
|
@jugglinmike, I realize that reverting the ansible script to change the permissions on |
|
A short-term solution might be to rely on filesystem ownership (or potentially group membership) rather than "world writability." In those tests above, the "write" bit is always set for both the directory's owner and group. That means if we configured Vagrant to set the owner or group of the synced folder to a more appropriate value (e.g. (For a longer-term solution, we might consider using the |
| config.vm.synced_folder '.', '/vagrant', type: 'rsync', | ||
| rsync__exclude: ['node_modules/', 'client/dist/'] | ||
| rsync__exclude: ['node_modules/', 'client/dist/'], | ||
| rsync__args: ['-r', '--perms', '--chmod=Du=rwx,Dg=rwx,Do=rwx,Fu=rwx,Fg=rwx,Fo=rwx', './server'] |
There was a problem hiding this comment.
@jugglinmike This is a naive world writable solution for the rsync options on the server folder. I know it's not setting the ownership on aria-bot, as suggested, but it does circumvent a Vagrant error on rsync when aria-bot isn't created on first provision. Given the time constraints, could we push this or something like this forward so that we can get the upgrade in place before the sprint ends?
There was a problem hiding this comment.
@jugglinmike I can also confirm that with this change, I am no longer running into permissions issues.
The directory is now made world-writable every time it is synchronized with the host.
There was a problem hiding this comment.
@jugglinmike I can also confirm that with this change, I am no longer running into permissions issues.
Yup, that corrects permissions for me, too:
$ vagrant up
[elided]
$ vagrant ssh -- ls -lahd /vagrant/server/scripts/import-tests
drwxrwxrwx 2 vagrant vagrant 4.0K Mar 7 21:14 /vagrant/server/scripts/import-tests
$ vagrant rsync
==> vagrant: Rsyncing folder: /home/mike/projects/bocoup/facebook-aria/aria-at-app/ => /vagrant
==> vagrant: - Exclude: [".vagrant/", "node_modules/", "client/dist/"]
$ vagrant ssh -- ls -lahd /vagrant/server/scripts/import-tests
drwxrwxrwx 2 vagrant vagrant 4.0K Mar 7 21:22 /vagrant/server/scripts/import-tests
$ vagrant reload
[elided]
$ vagrant ssh -- ls -lahd /vagrant/server/scripts/import-tests
drwxrwxrwx 2 vagrant vagrant 4.0K Mar 7 21:22 /vagrant/server/scripts/import-tests
More to the point: it allows the "import" task to complete successfully.
Given how much thought went into this, I'd like to see some documentation about the rationale for the configuration. Additionally, since it obviates the Ansible Task we've been maintaining, we should remove that. I've made those changes myself for expediency's sake. If they look good to you, then you can merge this!
|
@jugglinmike Thank you! I appreciate your thoughts and guidance on this task! |
* Update some major dependencies, webpack config and proxy management for "Open Test Page button" * Update icon packs * Webpack config changes * Update bootstrap * Update eslint & jest * Update connect-pg-simple * Update css * Remove query-string dependency * jest.setup.js update * Update runtest.yml to use Node v18 * Updating form text (#485) * Update dependencies test (#484) * Updating jest environment and babel configuration * New github config to fix builds * Upgrade lighthouse * Remove nodegit dependency * Address PR feedback * Fix files affected by revert on source branch * Update React * Fix fake data seeder * Remove sass dependency * Fix files affected by revert on source branch * Fix misspellings in the readme (#490) * Fix misspellings in the readme --------- Co-authored-by: Howard Edwards <howarde.edwards@gmail.com> * Update sequelize * Update gitUpdatedDateToString function arguments * Updating react deps (#502) * Update React * Updating jest environment and babel configuration * New github config to fix builds * Cacheing nodegit and fixing failing tests * update react in package.json * Updating React-Testing-Library and removing acts (default now) * adding text encoder, which is not added to jsdom * updating react testing library dom * Updating jest environment and babel configuration * update react in package.json * adding text encoder, which is not added to jsdom * cleaning up confirmAuth + candidateTests + routes index.js * descendent routing in progress * fixed candidate tests + test reports. TODO: test queue * fixing typo: propotypes --> proptypes * Undoing irrelevant commits * Undoing uneccesary whitespaces * Removing unnecessary imports * cleaning up confirmAuth + candidateTests + routes index.js * descendent routing in progress * undoing irrelevant commits * undoing uneccesary whitespaces * removing unnecessary imports * Update yarn.lock * report fixings * fixing jest test expected * removing unused import * updated runtest.yml * removing resize observer * confirmed test expected is different per machine, reverting * fixing resize-observer * check for null | undefined testplanreports * summarizetestplanversion reverts * Re-enabling test on App.test.jsx * updated testing library * adding yarn lock * yarn prettier * Removing comment --------- Co-authored-by: Howard Edwards <howarde.edwards@gmail.com> Co-authored-by: Erika Miguel <erika@bocoup.com> * Update rest of dependencies 2 (#503) * Upgrade prettier * update babel * Updating babel and prettier correctly * Upgrade ua-parser-js and lighthouse cli * Moving babel deps to dev dependencies * updating prettier config * prettier changes * Update sequelize-cli version (#508) * Update sequelize-cli version * Update dependencies * Updating More Dependencies (#506) * untested changes * fixed storybook * alphabetical * changing to dev deps * Implement fixes for support tables * Upgrade vagrant environment (#492) * Vagrant environment changes * adding memory increase and cron job * Automatically upgrade to Node 18 and fix cron job on Vagrant environment * Adding cores to Vagrant machine * Adding environment variable to differentiate between deployment environments in embed.js * Changing permissions to /vagrant/server folder * separating out upgrade tasks * explicitly adding patch-package to root * removing DEPLOY_ENVIRONMENT variable * remove DEPLOY_ENVIRONMENT from vagrant config * adding ALLOW_FAKE_ROLE env variable to vagrant env * World write rsync permission on server folder on vagrant machine * Document new Vagrant configuration * Remove obviated Ansible Task The directory is now made world-writable every time it is synchronized with the host. --------- Co-authored-by: Mike Pennisi <mike@mikepennisi.com> * Update yarn.lock --------- Co-authored-by: Erika Miguel <erika@bocoup.com> Co-authored-by: alflennik <alflennik@users.noreply.github.com> Co-authored-by: Paul Clue <67766160+Paul-Clue@users.noreply.github.com> Co-authored-by: Aleena <55119766+aleenaloves@users.noreply.github.com> Co-authored-by: Mike Pennisi <mike@mikepennisi.com>
This PR
To mimic an upgrade in Vagrant:
git checkout vagrant-main-node-14vagrant upgit checkout upgrade-vagrant-environmentvagrant reload --provisionand this task shows: