Added Get Domains by PolicyType functions #221
Conversation
api/gather.go
Outdated
| ) | ||
|
|
||
| // gatherLists returns a list of domains filtered by Policy types "bulk-18-weeks" and "bulk-1-year" | ||
| func gatherLists(list preloadlist.PreloadList) ([]string, []string) { |
There was a problem hiding this comment.
I can see that this might be useful, but as implemented this is vague and ambiguous in several ways:
- The return types are not annotated. It's very easy to mis-use them. I would suggest using a map or a struct.
- The function signature (and comment) do not indicate that this drops entries.
Also:
- This function is not capitalized, so it's not exported. But it's also not used internally. So it's not doing anything.
- Since the strings are constants, it would be good to define them as capitalized constants, ideally as an anum.
- Any reason to return just the domain strings rather than the entries?
- Any reason to implement this in the
hstspreload.orgpackage rather thanhstspreload?
I also don't see a stated motivation for this PR. If the goal is to be able to get subsets of the list, would it make just as much sense to provide filters for individual states rather than gathering some subsets at once?
There was a problem hiding this comment.
Hi Lucas,
Thanks so much for your comments they were very helpful. I'm currently a step intern working under @nharper and @carlosjoan91, and the project we are working on revolves around automating processes concerning the hsts preload list. I have attempted to make changes to the code that adheres to some of your suggestions by providing functions that filter for individual states rather than gathering some subsets at once, returning entries instead of strings (thereby annotating the return type), capitalizing the function so that it is exportable, and defining constants as enums. Initially, I chose to return just the domain strings rather than the entries because I thought we only needed the domain name to check whether the domain is preloadable with the PreloadableDomain() function. However, I forgot that to change the status of a domain, you also need the value of the includeSubDomains field, which led me to choose to return the entire entry. Moreover, my understanding as the reason this is implemented in the hstspreload.org package instead of the hstspreload is because we plan to use a task scheduling tool that will call the endpoint at regular intervals.
There was a problem hiding this comment.
Ah, thanks, I was missing the context! Happy to see work on this project, but I'm glad to leave you in @nharper and @carlosjoan91's capable hands for an actual review. 😄
t file, Changed Return Type, Added Constants
tKabiawu
changed the title
api/gather.go
Outdated
| type PolicyType int | ||
|
|
||
| const ( | ||
| bulk18Week PolicyType = iota | ||
| bulk1Year | ||
| ) | ||
|
|
||
| func (pt PolicyType) String() string { | ||
| return []string{"bulk-18-weeks", "bulk-1-year"}[pt] | ||
| } |
There was a problem hiding this comment.
PolicyType can be a string and then the slice isn't needed. If PolicyType is defined to be a string, then the preloadlist.Entry struct can have its Policy field be of type PolicyType. See the preload status field in DomainState as an example.
…ies, and used constants from hstspreload repository
tKabiawu
changed the title
This addition creates two functions that will get entries from a preload list according to the entries' policy type. These entires will be used in later additions to the hstspreload and hstspreload.org repositories that will allow us to automate some of the processes involved in removing lists that no longer adhere to the dynamic HSTS requirements. We put this code in the hstspreload.org repo, specifically in the api folder, so that in the future when the rest of the code is finished, we can use a tool that calls this endpoints at regular intervals. This function will likely be exported, but there is still some ambiguity in how it will be implemented until we move onto next steps. My reviewers are @nharper and @carlosjoan91