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

FEATURE: Update discourse-setup to prompt for MaxMind account ID #796

Merged

Conversation

tgxworld
Copy link
Contributor

@tgxworld tgxworld commented May 3, 2024

In order to download the free MaxMind GeoLite2 databases, an account ID
and license key is required going forward. This commit updates
discourse-setup to start prompting the user to provide the MaxMind
Account ID first before asking for the MaxMind license key. If the user
does not provide the Account ID, the script will not prompt for the
license key as we assume the user has opted out.

We are aware that we don't have a reliable way to test for changes to
the discourse-setup script but it is what it is at this point in time.
We intend to invest resources in improving things in the future but now
is not the time.

Review notes

We unfortunately have to test out the changes by hand:

  1. Clone this repo
  2. Run sudo SKIP_CONNECTION_TEST=1 ./discourse-setup
  3. Enter random values when prompted because the values don't matter.
  4. The script will print out "Unfortunately, there was an error changing containers/app.yml" but we can ignore it.
  5. Run sudo cat containers/app.yml | grep MAXMIND to see that the envs have been updated to the values prompted before.

@tgxworld tgxworld requested a review from nbianca May 3, 2024 02:34
@tgxworld
Copy link
Contributor Author

tgxworld commented May 3, 2024

@pfaffman Pinging you for visibility since you added the MAXMIND config 3 years back.

discourse-setup Outdated
then
cat <<EOF

Adding DISCOURSE_MAXMIND_LICENSE_KEY to $web_file has failed! This
Adding DISCOURSE_MAXMIND_ACCOUNT_ID and DISCOURSE_MAXMIND_LICENSE_KEY to $web_file has failed! This
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: I think this line should be 80 characters long like the ones below.

@pfaffman
Copy link
Contributor

pfaffman commented May 3, 2024

Hey! Thanks for pinging me @tgxworld ! I've been meaning to take a look at this today but the day got away from me.

Have a look at tests/README.md. It's not real tests, but it's better than nothing. You don't have to run all the stuff by hand, at least, not each one by hand.

@tgxworld
Copy link
Contributor Author

tgxworld commented May 6, 2024

Have a look at tests/README.md. It's not real tests, but it's better than nothing. You don't have to run all the stuff by hand, at least, not each one by hand.

I think the tests are all broken at this point, will you be able to have a look? If not, I might want to drop them instead since we have plans to revamp the launcher and discourse-setup scripts.

@pfaffman
Copy link
Contributor

pfaffman commented May 6, 2024

It could be that something changed in what the script expects since I wrote those tests. If they don't help you, then they don't help you.

It looks like values are getting set, but when it runs a second time to see if it can change already saved values, it's failing. I'm not sure how or why that would be. Ah. Here it is:

In tests/standalone:

-$DIR/../discourse-setup --skip-connection-test --skip-rebuild <<EOF
+$DIR/../discourse-setup --debug --skip-connection-test --skip-rebuild <<EOF

In discourse-setup:

then
echo "Stopping existing container in 5 seconds or Control-C to cancel."
sleep 5

  • ./launcher stop $app_name
    else
    echo "DEBUG MODE ON. Not stopping the container."
    fi
  • ./launcher stop $app_name
    assert_maxmind_license_key
    assert_notification_email

InEnglish: Previously, the launcher stop was not in the if. Also, the test script needs to call ./discourse-setup --debug --skip-connection-tests --skip-rebuild (--debug was missing). I guess that stuff got added after I last touched the tests?

Adding DEBUG=1 before it runs the second time to change settings would speed things up. But there's still something that keeps the edits from getting processed.

It's a bummer that 1234567890123456 was decided as the placeholder for not having a maxmind key.

You can check out assert_maxmind_license_key to see how to add the new required setting to files that don't have them.

I might be inclined to remove support for maxmind from discourse-setup altogether and assume that people who can configure maxmind can manage to use vim/nano to do it by hand.

I hope this helps a bit.

I'm about to leave town for a couple of weeks and don't have much time to help out. If I did, I'd try to submit a proper PR to fix the test, but I'm horrible at git.

OK. I did rookie edits in the github web interface and submitted it as two PRs. I fixed only the standalone script but assume the others need the same fix.

@tgxworld tgxworld force-pushed the update_discourse_setup_to_request_for_maxmind_account_id branch from 694f900 to c199633 Compare May 9, 2024 07:39
In order to download the free MaxMind GeoLite2 databases, an account ID
and license key is required going forward. This commit updates
`discourse-setup` to start prompting the user to provide the MaxMind
Account ID first before asking for the MaxMind license key. If the user
does not provide the Account ID, the script will not prompt for the
license key as we assume the user has opted out.

We are aware that we don't have a reliable way to test for changes to
the `discourse-setup` script but it is what it is at this point in time.
We intend to invest resources in improving things in the future but now
is not the time.
@tgxworld tgxworld force-pushed the update_discourse_setup_to_request_for_maxmind_account_id branch from c199633 to 434e4f6 Compare May 9, 2024 07:39
@tgxworld tgxworld merged commit 3596dc1 into main May 9, 2024
2 checks passed
@tgxworld tgxworld deleted the update_discourse_setup_to_request_for_maxmind_account_id branch May 9, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants