Migrate Gallery.CredentialExpiration to JsonConfig #517
Conversation
chenriksson
requested review from
joelverhagen,
agr,
loic-sharma,
shishirx34,
ryuyu,
cristinamanum,
skofman1 and
scottbommarito
chenriksson
force-pushed
the
chenriks-maint-config
branch
from
9b9fe95 to
5987d6f
Compare
chenriksson
force-pushed
the
chenriks-credexp-config
branch
from
88bdef2 to
e3b6c61
Compare
|
|
||
| namespace Gallery.CredentialExpiration | ||
| { | ||
| public class InitializationConfiguration |
There was a problem hiding this comment.
IMO having classes with the same name in different projects makes things difficult to work with.
When every project has a class with the same name it becomes impossible to CTRL-T to any particular instance of the file.
Can we rename this to CredentialExpirationConfiguration or something, and do a similar thing for all of the other jobs when you migrate them?
(pending team discussion)
There was a problem hiding this comment.
Thanks for the suggestion - if consensus is to rename, I'll address in follow-up PR.
| { | ||
| public class InitializationConfiguration | ||
| { | ||
| public int AllowEmailResendAfterDays { get; set; } |
There was a problem hiding this comment.
maybe not totally essential for this PR but I think it's always very helpful when config files have detailed XML docs for what each config does
/// <summary>
/// After the job has sent an email about the expiration of a credential, it will wait this long before sending another email.
/// </summary>
public int AllowEmailResendAfterDays { get; set; }
There was a problem hiding this comment.
@scottbommarito - I'll do a follow-up PR to improve doc comments across all the jobs I'm updating.
There was a problem hiding this comment.
Will also add readme.md for each job to help w/ onboarding - per last sprint's retrospective. :-)
| @@ -65,6 +66,9 @@ | |||
| <SubType>Designer</SubType> | |||
| </None> | |||
| <None Include="Scripts\*" /> | |||
| <None Include="Settings\dev.json" /> | |||
There was a problem hiding this comment.
Settings\* or Settings\*.json is easier to maintain. It would also match the nuspec better. Additionally, if the Jobs.Common implementation allows you to specify any file, using a glob would show all possible files.
There was a problem hiding this comment.
Note that Settings\* will be moving out of NuGet.Jobs and into NuGetDeployment as part of the jobs deployment work... for this reason, going to not fix this.
|
@joelverhagen Updated to use IOptionsSnapshot: b3cd012 |
Part of https://github.com/NuGet/Engineering/issues/1560. Depends on #505
Status: Verified on DEV