Skip to content

fix: Allow empty paths in SecretsLoaderHelper #2250

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

Merged
merged 2 commits into from
Oct 12, 2022

Conversation

simon-lemay-unity
Copy link
Contributor

If a path was not provided, it would throw an exception while loading the (non-existent) file. But this will be the normal use case in live deployments, since servers and clients will have different files to load (e.g. the client will not be loading a private key).

This PR makes it so that not providing a path will just mean an empty string is used as the contents of the file. This should be functionally equivalent to not providing these values at all.

This PR also makes the paths empty by default. The previous defaults made sense in the context of the script in the test project that generates the files, but they are unlikely to be valuable to users otherwise. Not having any defaults also means we don't seem like we're taking position as to where certificates must be stored (having defaults pointing to the assets folder could have led users to think this was good or standard practice).

Changelog

N/A

Testing and Documentation

  • No tests have been added.
  • No documentation changes or additions were necessary.

@simon-lemay-unity simon-lemay-unity requested a review from a team as a code owner October 11, 2022 21:07
@jeffreyrainy
Copy link
Contributor

I don't mind the change. But, I think the exception was known and caught by

        private void Awake()
        {
            var serverSecrets = new ServerSecrets();

            try
            {
                serverSecrets.ServerCertificate = ServerCertificate;
            }
            catch (Exception exception)
            {
                Debug.Log(exception);
            }

@simon-lemay-unity
Copy link
Contributor Author

@jeffreyrainy Yes, you're absolutely right. My PR description is inaccurate. I should have written that it only logs the exception (which is an order of magnitude less problematic than outright throwing). I guess my point was more that the behavior was non-optimal for a use case which should occur for a lot of deployments.

@simon-lemay-unity simon-lemay-unity merged commit adc06f6 into develop Oct 12, 2022
@simon-lemay-unity simon-lemay-unity deleted the fix/secrets-loader-empty-paths branch October 12, 2022 02:15
jakobbbb pushed a commit to GooseGirlGames/com.unity.netcode.gameobjects that referenced this pull request Feb 22, 2023
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.

4 participants