Skip to content

DOCSP-45732-add-proxy-setting-compass-config #716

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

Conversation

amalhotra-mdb
Copy link
Contributor

@amalhotra-mdb amalhotra-mdb commented Jan 27, 2025

DESCRIPTION

add a new proxy setting to the Compass config file docs.

STAGING

https://deploy-preview-716--docs-compass.netlify.app/settings/config-file/

JIRA

https://jira.mongodb.org/browse/DOCSP-45732

BUILD LOG

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Is this free of spelling errors?
  • Is this free of grammatical errors?
  • Is this free of staging / rendering issues?
  • Are all the links working?

External Review Requirements

What's expected of an external reviewer?

Copy link

netlify bot commented Jan 27, 2025

Deploy Preview for docs-compass ready!

Name Link
🔨 Latest commit d03a786
🔍 Latest deploy log https://app.netlify.com/sites/docs-compass/deploys/679a4f9fa9e8990008b73f5e
😎 Deploy Preview https://deploy-preview-716--docs-compass.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

with. For more information on connecting to your deployment with a
proxy, see :ref:`ssh-connection`.

You must pass either a URL or a DevtoolsProxyOptions object as JSON

Choose a reason for hiding this comment

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

Users won't know what a DevtoolsProxyOptions object is -- we'll need to explain what this means, or maybe give an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for your review, @betsybutton ; changes made, let me know how the approach i took looks to you.

Comment on lines 135 to 136
for the Proxy setting. You can also set the Proxy setting to an empty
string.

Choose a reason for hiding this comment

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

Can we specify that if proxy is set to an empty string, no proxy will be used?

proxy, see :ref:`ssh-connection`.

You must pass either a URL or a ``DevtoolsProxyOptions`` object as JSON
for the Proxy setting. A ``DevtoolsProxyOptions`` object takes the following
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a mixture of proxy with and without capital P here, might be good to align them (I see no reason why we'd capitalize it)

Comment on lines 143 to 149
useEnvironmentVariableProxies: boolean;
sshOptions: {
identityKeyFile: string;
identityKeyPassphrase: string;
};
ca: ConnectionOptions['ca'];
env: Record<string, string | undefined>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
useEnvironmentVariableProxies: boolean;
sshOptions: {
identityKeyFile: string;
identityKeyPassphrase: string;
};
ca: ConnectionOptions['ca'];
env: Record<string, string | undefined>;

I wouldn't expose any of these options to users, they're meant to be internal. proxy and noProxyHosts are the relevant options here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for taking a look at this @addaleax, could you take a second look at the changes, when you get a chance?

with. For more information on connecting to your deployment with a
proxy, see :ref:`ssh-connection`.

You must pass either a URL or a ``DevtoolsProxyOptions`` object as JSON
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd still avoid using DevtoolsProxyOptions here if we can. It's not a meaningful term for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @addaleax ! just to clarify, do you think we should take out the references to DevtoolsProxyOptions and the JSON block showing the format for the object, or just take out the references to DevtoolsProxyOptions but keep the code block?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's just the name that's meaningless, the JS block (it's JS here, not JSON, although the users will need to use JSON – maybe also worth adjusting) defines what the structure is so there's no need to define a name that's never used again

Copy link

@betsybutton betsybutton Jan 28, 2025

Choose a reason for hiding this comment

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

Agreed - it would be better to have the example represent the actual structure the value needs to take.

Comment on lines 130 to 131
- Allows you to enforce or define a proxy to connect to your deployment
with. For more information on connecting to your deployment with a

Choose a reason for hiding this comment

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

Super nit pick, but ending a sentence with "with" sounds a bit informal. Should we reword this to:

Enables you to specify or enforce the use of a proxy for connecting to your deployment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @betsybutton ! could you take a final look at this to see if the changes look ok to you?

Copy link
Collaborator

@ltran-mdb2 ltran-mdb2 left a comment

Choose a reason for hiding this comment

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

Thanks @amalhotra-mdb ! I just left a couple suggestions!

Comment on lines 130 to 131
- Enables you to specify or enforce the use of a proxy for connecting
to your deployment. For more information on connecting to your
Copy link
Collaborator

Choose a reason for hiding this comment

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

[suggestion] to avoid using a gerund

Suggested change
- Enables you to specify or enforce the use of a proxy for connecting
to your deployment. For more information on connecting to your
- Enables you to specify or enforce the use of a proxy to connect
to your deployment. For more information on connecting to your

Comment on lines 134 to 135
You must pass either a URL or pass the following format:

Copy link
Collaborator

Choose a reason for hiding this comment

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

[suggestion] for minimalism

Suggested change
You must pass either a URL or pass the following format:
You must pass either a URL or the following format:

Comment on lines 143 to 144
If you set ``proxy`` to an empty string, no
proxy will be used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[suggestion] for active voice

Suggested change
If you set ``proxy`` to an empty string, no
proxy will be used.
If you set ``proxy`` to an empty string, your deployment does not use a proxy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want this, it should be

Suggested change
If you set ``proxy`` to an empty string, no
proxy will be used.
If you set ``proxy`` to an empty string, Compass does not use a proxy.

@amalhotra-mdb
Copy link
Contributor Author

Thanks for your review, @ltran-mdb2 ! Could you take another look at this, whenever you get a chance? Thank you!

Copy link
Collaborator

@ltran-mdb2 ltran-mdb2 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @amalhotra-mdb !

Copy link

@betsybutton betsybutton left a comment

Choose a reason for hiding this comment

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

LGTM, thank you Anika!

@jeff-allen-mongo jeff-allen-mongo merged commit 7710cb9 into mongodb:master Jan 31, 2025
5 checks passed
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