Updated mdmctl to allow base server URL to have a path#683
Updated mdmctl to allow base server URL to have a path#683groob merged 3 commits intomicromdm:mainfrom
Conversation
…ommit. Added unit tests that confirm serverURL's with paths work as expected and that invalid serverURL's return an error.
|
What's the reason for the trailing slash to be required? |
cmd/mdmctl/config.go
Outdated
| } | ||
| u.Path = "/" | ||
| serverURL = u.String() | ||
| const tempPath = "x" |
There was a problem hiding this comment.
I would appreciate a slightly longer comment here explaining why a temporary path is joined and then trimmed.
There was a problem hiding this comment.
Actually, the trailing slash is not required. I assured the slash was there so the existing unit tests would pass and to keep existing behavior consistent. If you are ok with it, I can remove the code that assures the trailing slash is there and adjust the unit tests to expect that in some cases there will be no trailing slash.
Alternatively, I can keep the code that assures the trailing slash is there and provide a better comment.
Thank you @groob.
There was a problem hiding this comment.
I tested removing the code that assures the url ends with a slash. In that state, if a trailing slash is provided, it is kept. If a trailing slash is not provided, it is not added.
With or without a trailing slash, the CopyURL() method correctly appends paths to the server url.
There was a problem hiding this comment.
Sounds good. I would favor not enforcing the slash, since the behavior of path.Clean in go is to remove any trailing slashes.
I was also concerned that enforcing a trailing slash would affect existing user's configs, but I then realized that the only existing support was for the / at the root. so we're fine.
There was a problem hiding this comment.
As we discussed, I removed code in the validateServerURL method that assured the URL ends in a slash and adjusted the unit tests to expect that in some cases there will be no trailing slash.
Let me know if you'd like me to submit a new pull request with this change as just one commit. I am new to pull requests and am not sure if you can or would want to combine these three commits into one.
Thank you.
There was a problem hiding this comment.
You can rebase and force push a clean single commit to your pull request.
Or, I can just squash the commit and use a commit message you give me/I write for you. The effect is the same, we always close pull requests with a single commit.
There was a problem hiding this comment.
Thanks for the explanation @groob. Please squash the commits for me since I am just learning rebasing. Here is a starting point for a single commit message...
Updated mdmctl to allow server-url to have a path, for example: "https://www.example.com/test". Prior to this, any path component was removed. The example url would have been modified to "https://www.example.com/".
…s in a slash. Adjusted the unit tests to expect that in some cases there will be no trailing slash.
2 participants
Updated mdmctl to allow its base server URL to have a path. For example:
https://www.example.com/test/The first MicroMDM server I configured required that its base URL have a path. When I set my local mdmctl server url with a path, I didn't realize mdmctl removed it. My commands failed with confusing errors. For example,
mdmctl get devicesreturned "invalid character 'O' looking for beginning of value".It took me a little while to figure out the server url path was removed. Then, I tried editing servers.json directly and added the server path. mdmctl still did not work because it removed the path again after the server URL was loaded from servers.json. This exercise took some time troubleshooting. I'd like to prevent this from happening to others.