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

Pass credentials around with context #203

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jun 1, 2024

Overview

My higher goal is to have State not store the workspace, context, nor credentials. See #210

We store Context and Credentials separately in the database. So it makes sense to keep them separate.

But having a single container that holds all runtime state simplifies matters.

Before

Passed Context and Credentials separately.
The State accessed them using State#workspace#context and State#workspace#credentials.

If we go towards removing State#workspace, run_nonblock(context, credentials) will provide that data.

After

Context contains the context and Context#to_h returns the same value. But Context#credentials will contain the sensitive credentials.

The State accesses them using State#workspace#context and `State#workspace#context#credentials.

If we go towards removing State#workspace, run_nonblock(context) will be able to provide that data.

Compromise

Would have liked to remove Workspace#context and Workspace#initialize(..., context), but left it in there until we update manageiq-providers-workspace.

@kbrock kbrock added the enhancement New feature or request label Jun 1, 2024
@kbrock kbrock requested review from agrare and Fryguy as code owners June 1, 2024 05:18
@kbrock kbrock added refactoring and removed enhancement New feature or request labels Jun 3, 2024
@kbrock
Copy link
Member Author

kbrock commented Jun 3, 2024

update:

  • changed interface to be backwards compatible

@kbrock
Copy link
Member Author

kbrock commented Jun 5, 2024

update:

  • rebased

Context is not part of serialized context. context.credentials is treated separately
But passing them around together simplifies the process
@kbrock
Copy link
Member Author

kbrock commented Jun 12, 2024

update:

  • rebase

@miq-bot
Copy link
Member

miq-bot commented Jun 12, 2024

Checked commit kbrock@deffd73 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
9 files checked, 0 offenses detected
Everything looks fine. 🍰

@agrare agrare merged commit e967a86 into ManageIQ:master Jun 13, 2024
5 checks passed
@kbrock kbrock deleted the context_credentials branch June 13, 2024 17:33
agrare added a commit that referenced this pull request Jun 20, 2024
Fixed
- ResultPath=$ replaces complete output (#199)
- Fix retrier backoff values (#200)
- Fix Retry issues (#202)
- Add Apache-2.0 license (#217)

Changed
- Update gemspec summary (#205)
- Simpler State#long_name (#204)
- State only modifies Context#state - prep for Map/Parallel (#206)
- Set StateHistory in Workflow not State (#211)
- Make Runner#wait optional (#190)
- Pass credentials around with context (#203)
- Pass context to State without workflow (#216)
- Move the guts of the CLI into a class for easy testing (#220)

Added
- Set State PreviousStateGuid in StateHistory (#208)
- Add a codeclimate config file (#224)
- Add an Execution unique ID to Context (#226)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants