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

Support credentials parameters in the Scripting API #536

Merged
merged 5 commits into from
Nov 25, 2020

Conversation

LinqLover
Copy link
Collaborator

@LinqLover LinqLover commented Nov 7, 2020

With this PR, two new parameters are added to the Metacello Scripting API, #username and #password, which allow overriding the credentials for the specified fetch operation temporarily. Example usage:

Metacello new
	baseline: 'External';
	repository: 'github://LinqLover/MetacelloTest-External/repository';
	username: 'myfancyusername';
	password: 'mycoolpassword';
	load.

The new parameters should at least work for GitHub + BitBucket repositories using Squeak (see also MetacelloRepositorySqueakCommonTestCase >> testGetGithubRepositoryScriptingAPI), but HTTP/FTP repositories are expected to work as well (though I did not test it). Other Smalltalk platforms are not supported at the moment; analogously to #534 (comment), I'd propose adding support for them on-demand only. (I have no how Iceberg works (not to speak of Gemstone at all), so this would probably go beyond my skills at the moment ...)

Looking forward to your review! :-)

@dalehenrich
Copy link
Member

@LinqLover on the off chance that you have more changes planned, I'll wait for a confirmation from you that you're done before merging ...

@LinqLover
Copy link
Collaborator Author

LinqLover commented Nov 8, 2020

@dalehenrich Thanks for your review! What does not yet work is specifying credentials for the loading of entire dependency trees, e.g. given a private GitHub package A that requires a private GitHub package B via baseline. I'm not yet sure how this would be best implemented, probably via another global variable plus an additional #recursiveAuthArg in the Metacello Scripting API, but I do not yet have a proper overview of all authenticable sources that can be used with Metacello (see #538 (comment)).

However, I think this change would fit well into a second PR, so unless you have further objections, please feel free to merge this one! :-)

@dalehenrich
Copy link
Member

@LinqLover, before pulling the trigger on this ... have you seen that github is deprecating password authentication ... I can go ahead and merge this PR, but if github will be deprecating password authentication, it may be that it is a little late to add this support? I have not looked into the replacements: personal access tokens or the web application flow ...

@LinqLover
Copy link
Collaborator Author

Ah yes, I was already aware of this ... It is only a naming issue. You can pass an access token as #password as well (which even works without specifying a username). I did not want to rename the selectors for reasons of compatibility/uniformity. Do you think I should add some explaining comments about access tokens?

@dalehenrich
Copy link
Member

@LinqLover comments would be a good idea ... I didn't know the deal with access tokens, but now I do:)

@LinqLover
Copy link
Collaborator Author

LinqLover commented Nov 16, 2020

Sorry for the long delay ... Now there are finally comments. :-)

PS: I just noticed that git apparently has converted some line endings because I was committing using Windows. Is this a problem for filetree? Otherwise, PR is ready from my side!

@LinqLover
Copy link
Collaborator Author

@dalehenrich Any updates on this? I see I have repository access (thank you!), but I would prefer only to merge this after you have approved the latest changes. :-)

@dalehenrich dalehenrich merged commit f336f66 into Metacello:master Nov 25, 2020
@LinqLover LinqLover deleted the feat-scripting-password branch November 25, 2020 17:02
@LinqLover
Copy link
Collaborator Author

Thank you! :-)

LinqLover added a commit to LinqLover/smalltalkCI that referenced this pull request Nov 25, 2020
Because now Metacello/metacello#536 has been merged! 🎉
@dalehenrich
Copy link
Member

@LinqLover I'm glad to see you contribute to the project and it is much appreciated ... BTW, with regards to triggering a CI build, in case you didn't know, there is a more options menu on the travisCI site for each project that you can use to trigger a build for given branch and custom commit comment ... saves a trigger ci commit:) ... I used that feature to trigger this travis build

@LinqLover
Copy link
Collaborator Author

Yes, I knew this trigger build button - pretty helpful, I already used it for this PR! But in this case (LinqLover/smalltalkCI@f63a564) I needed to retrigger the CI for my PR at hpi-swa/smalltalkCI for which I don't have administrative access rights to Travis ... :-)

fniephaus added a commit to hpi-swa/smalltalkCI that referenced this pull request May 16, 2021
* SCIMetacelloLoadSpec: #username and #password

Complements Metacello/metacello#536.

* Make sure to install latest Metacello when credentials params are used

See #485 (comment).

* Add load config params #userEnv and #passwordEnv

These new parameters make it possible to provide secret credentials via
environment variables instead of hard-coding them into the configuration
file. At the moment only supported by SCIGoferLoadSpec and
SCIMonticelloLoadSpec, support will be added for SCIMetacelloLoadSpec
after #485 has been merged.

This change was also requested in
#485 (comment).

* Revise changes in README.md [ci-skip]

* Fix another small slip in the README.md [skip-ci]

* Trigger CI

* Trigger CI

Because now Metacello/metacello#536 has been merged! 🎉

* Only use authentication selectors if needed

Backward compatibility ...

* Update README.md: Document #{username,password}Env

* Factor out SCIAbstractLoadSpecWithCredentials

* Merge remote-tracking branch 'upstream/master' into feat/credentials-env-params

Manual rebased monticello.meta files, let's hope it works 😅

* Revert "Update README.md: Document #{username,password}Env"

Sorry, this should be part of #485 instead. This reverts commit 48d54b7.

* SCIAbstractLoadSpecWithCredentials: Fall back to nil instead of '' for empty credential values.

See: #485 (comment)

* Partially revert SmalltalkCI-Core-ct.264: Monticello & Gofer do not tolerate nil credentials indeed.

However, keep the nil return value in SCIAbstractLoadSpecWithCredentials in preparation for SCIMetacelloLoadSpec (see #485).

* Corrects merge error from SmalltalkCI-Core-ct.268 and fixes compatibility-aware behavior of 80ae119. Optimized.

* Revert "Revert "Update README.md: Document #{username,password}Env""

This reverts commit 22811cf.

* Merge branch 'feat/credentials-env-params' into feat/metacello-credentials

* Revert "Revert part of e777ff5 that did not belong here"

This reverts commit ff77144.

* Minor edit

Co-authored-by: Fabio Niephaus <code@fniephaus.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants