Skip to content
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-labels.sh isn't ready for general use #71

Open
pixelzoom opened this issue Feb 7, 2018 · 7 comments
Open

add-labels.sh isn't ready for general use #71

pixelzoom opened this issue Feb 7, 2018 · 7 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 7, 2018

See #69 (comment).

I spend > 1 hour trying to get add-labels.sh running and failed. I did this because in #69 (comment), @mattpen said that developers should be running it as part of the new sim check list. But this script really doesn't feel like it's ready for general use.

On macOS:

% ./add-labels.sh phetsims/equality-explorer
./add-labels.sh: line 14: [[: phetsims/equality-explorer: division by 0 (error token is "-explorer")
...

And then it continues to run.

I also had problems when I had a typo in the $1 arg (e.g. phetsims/equality-explore), when my credentials were incorrect, etc.

Problems:
(1) Overall lack of error handling
(2) Unnecessary output that should be redirectly to /dev/null
(3) Some errors are apparently expected, per this Slack conversation with @mattpen:

Chris Malley [11:46 AM]
What am I doing wrong here?

% ./add-labels.sh phetsims/equality-explorer
...
{"message":"Not Found","documentation_url":"https://developer.github.com/v3"}

Same message over and over.

Matt Pennington [11:48 AM]
That should happen the first 10 attempts or so, then you should get an error that says "Already Exists" or something similar.

If this is a lot of work, I'm OK with deferring, and having @mattpen handle updates.

@pixelzoom
Copy link
Contributor Author

Also from #69 (comment):

GitHub credentials are stored in ~/.phet/.credentials, and information about it is in a public README file?!? That's a security hole just waiting to be exploited.

Highly recommended to use a more secure method of handling GitHub credentials. And if it does involves a file, make the filename more specific (e.g. github-credentials).

@mattpen
Copy link
Contributor

mattpen commented Feb 7, 2018

@ariel-phet - Can you please assign priority?

@ariel-phet
Copy link
Contributor

For the moment, @mattpen is quite busy with other things on the website, but generally good to have such scripts in "ready for prime-time" shape so anybody can run them.

@mattpen will see if this is something Jake can handle. If not he will assign back to me.

@ariel-phet ariel-phet removed their assignment Feb 7, 2018
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Feb 7, 2018

When this gets addressed, a few other suggestions:

• Test for prerequisite files. This script requires ~/.phet/.credentials and github-labels.json, but doesn't check that either exist. This results in hard failure on lines 5 and 64 respectively.

• Don't require navigating to phet-info/github-labels/ in order to run the add-labels.sh. Like the scripts in perennial/bin/, one should be able to put this directory in PATH and run it from anywhere. Locate github-labels.json the same way that perennial scripts locate active-repos. See for example clone-missing-repos.sh.

• Replace ~/.phet/.credentials with a more secure method of getting credentials. If that's not possible, rename to ~/phet/github-credentials, so that it's not as vague.

• Verify that the repo specified by $1 exists.

• Handle errors wherever they might occur. Recover when appropriate, halt when fatal.

• Send extraneous output to /dev/null. The current output is overwhelming, difficult to tell what's relevant and what's not.

• Use lowercase for variable names. Uppercase is typically used for system variables in shell scripts.

• Probably similar changes in change-label.sh and update-label.sh.

@mattpen
Copy link
Contributor

mattpen commented Feb 14, 2018

@jjohnson5253 said he would be interested in working on this.

To add to the list above, we should also consider using active-repos instead of phetsims-repos.json.

• Replace ~/.phet/.credentials with a more secure method of getting credentials. If that's not possible, rename to ~/phet/github-credentials, so that it's not as vague.

Perhaps we would require credential input when the script is run rather than relying on a plain text file. I'm not sure why this is an issue because we are storing credentials in plain-text in ~/.phet/build-local.json

@mattpen
Copy link
Contributor

mattpen commented Oct 30, 2018

Replace ~/.phet/.credentials with a more secure method of getting credentials. If that's not possible, rename to ~/phet/github-credentials, so that it's not as vague.

Use lowercase for variable names. Uppercase is typically used for system variables in shell scripts.

Send extraneous output to /dev/null. The current output is overwhelming, difficult to tell what's relevant and what's not.

These concerns were addressed in the above commit.

@mattpen
Copy link
Contributor

mattpen commented Aug 7, 2020

The script is working fine and has been used by many developers. Error handling would be nice to add, but currently isn't a priority. Removing assignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants