-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add debug symbols upload #59
Conversation
bc1fa8b
to
22e4db0
Compare
return $ini | ||
} | ||
|
||
$ConfigIni = ParseIniFile "$ConfigPath\DefaultEngine.ini" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file normally under version control? If so, we shouldn't use it for sentry CLI configuration (a.k.a sentry.properties
) as that's supposed to be excluded from git due to the auth-token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, DefaultEngine.ini
is an essential part of any UE project that keeps all of its settings (including plugins) and usually it's under source control. Though I don't think it can cause any issues for plugin users since they are likely to take care of securing their project config files. Or did I get it wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it's in a private repo, unless it's a single-person project, it's best not to commit auth tokens into source control. Additionally, this disqualifies any open-source games.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest sticking to a single sentry.propertiies
file, regardless of the platform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have the exact same issue with Unity then? AFAIK there auth token is stored in a separate asset (SentryCliOptions
scriptible object) which apparently could be a part of the game as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we use the sentry.properties file on windows without an issue with Unity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Practical illustration why we shouldn't store the settings in the same file as everything else - from the in-repo sample PR (#64):
How can we move that to a different location so the "shared" file doesn't change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we move that to a different location so the "shared" file doesn't change?
We can replace user credentials here with a path to sentry.properties
file which won't be publicly accessible. Later we can add some editor tool that will automate the creation of this file. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, no more user credentials in DefaultEngine.ini
. Now in order to obtain auth token and other stuff for Sentry CLI path to sentry.properties
file has to be provided firs. Apparently not an ideal solution since it may vary on different machines, though at least we've removed the risk of compromising sensitive data
|
@tustanivsky for Android, we need to update the UPL file - how can we reference the ProjectDir and the Sentry.PropertiesFilePath here?
|
It actually looks like something went wrong during recent merges - |
fixed please have a look at my changes and feel free to merge. Also, can you please manually check macOS? I've only tested windows & android with these changes |
Resolves #47
Resolves #57
Resolves #60
It will also make sense to automate Sentry CLI update in a way similar to other plugin dependencies (#60)