search

ossf / staff

Repository to keep track of staff operations
Apache License 2.0
0 stars 1 forks source link

Bug fix to project repo onboarding #6

Closed Danajoyluck closed 1 month ago

Danajoyluck commented 1 month ago

Updated the code to fix a bug that caused the HTTP response code not properly returned. Also cleaned up the code to make terminal output cleaner

david-a-wheeler commented 1 month ago

I made a bunch of comments. Many are minor style nits, but I have a few concerns about the JSON.

I suggest running "shellcheck" on the shell scripts, and a JSON validator on the JSON. Are the JSON files correct?

Danajoyluck commented 1 month ago

I made a bunch of comments. Many are minor style nits, but I have a few concerns about the JSON.

I suggest running "shellcheck" on the shell scripts, and a JSON validator on the JSON. Are the JSON files correct?

Thanks a lot @david-a-wheeler for the review....agree with all the recommendations.

do you feel those are blockers for the SIG repo to be provisioned with the current code base? Do you think I can make updates after the repo is provisioned? The results output file is named JSON for text editors to render the contents easily in JSON format where applicable.

Several security baseline work is being blocked by the lack of the repo. I'm hoping we can make the code perfect after removing the security baseline SIG blocker.

The goal of this code is to automate manual work. I hope this code is good enough for us t move forward, and leave room for improvement down the road.

david-a-wheeler commented 1 month ago

If the ".json" file isn't really JSON, but it's named that way to simplify tooling, I suggest at least adding a comment somewhere easily visible to document that. Otherwise, some tools will strongly object to this formatting. You could turn them into JSON5 comments and label them JSON5, too.

As far as the shell code goes, I don't think they're serious, so I don't think they are necessary. That said, in general you should use shellcheck to check on shell scripts. More specifically, I suggest using "$..." everywhere in shell scripts, it eliminates many mysterious bugs. It wouldn't take long to fix those few cases of missing "..." at least.

david-a-wheeler commented 1 month ago

I'd like for my comments to be addressed at some point, but none are serious & I don't want them to become blockers for this important item. So I'll approve. Dana - if you could revisit later please.

Note: JSON5 would be a good alternative. Use ".json5" as extension, make comments beginning with "// ".