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

Override config location with environment variable #161

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

ashcrow
Copy link
Contributor

@ashcrow ashcrow commented Feb 24, 2021

KEYLIME_CONFIG can be set to override the default config file location.

Note:
I think we should eventually rework the configuration system so that we load the configuration once and then reload when we receive the proper signal. My reasoning is that the current set up means that if the file changes while the agent is running then the result could be old and new variables in use based on when they have been read. I've started to make structs for the keylime config. If folks agree I'll slowly but surely chip away at that no matter if this merges or not 😄

@ashcrow ashcrow changed the title Override config location with environment variable WIP: Override config location with environment variable Feb 24, 2021
@ashcrow ashcrow force-pushed the config-via-env-var branch 5 times, most recently from dd2ae45 to 2db5684 Compare February 24, 2021 19:21
@ashcrow ashcrow changed the title WIP: Override config location with environment variable Override config location with environment variable Feb 24, 2021
@mpeters mpeters removed their assignment Feb 25, 2021
Copy link
Contributor

@lkatalin lkatalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be a great improvement; thank you for working on this!

src/common.rs Outdated Show resolved Hide resolved
@ashcrow ashcrow force-pushed the config-via-env-var branch from 2db5684 to 9bc6244 Compare March 2, 2021 15:42
@ashcrow
Copy link
Contributor Author

ashcrow commented Mar 2, 2021

Hrm the failure looks like code I didn't touch ...

114 | / pub(crate) fn run_revocation_actions(
115 | |     json: Value,
116 | | ) -> Result<Vec<Result<Output>>> {
117 | |     #[cfg(not(test))]
...   |
152 | |     Ok(outputs)
153 | | }
    | |_^

I'm willing to look at it deeper if required though.

@lkatalin
Copy link
Contributor

lkatalin commented Mar 2, 2021

Hrm the failure looks like code I didn't touch ...

114 | / pub(crate) fn run_revocation_actions(
115 | |     json: Value,
116 | | ) -> Result<Vec<Result<Output>>> {
117 | |     #[cfg(not(test))]
...   |
152 | |     Ok(outputs)
153 | | }
    | |_^

I'm willing to look at it deeper if required though.

Looks like the failure is related to the revocation scripts (not sure why clippy didn't catch this on the earlier PR). I'm making a fix for this and then you can rebase on that. The changes LGTM!

@lkatalin
Copy link
Contributor

lkatalin commented Mar 2, 2021

I went down a bit of a rabbit hole, but #162 should help once it is merged.

@ashcrow ashcrow force-pushed the config-via-env-var branch from 9bc6244 to 2be3cb9 Compare March 3, 2021 14:40
@ashcrow
Copy link
Contributor Author

ashcrow commented Mar 3, 2021

Rebased on #162

@ashcrow
Copy link
Contributor Author

ashcrow commented Mar 3, 2021

Woo! All green!

Copy link
Contributor

@lkatalin lkatalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great : )

KEYLIME_CONFIG can be set to override the default config file
location.

Signed-off-by: Steve Milner <[email protected]>
@ashcrow ashcrow force-pushed the config-via-env-var branch from 2be3cb9 to 6d69d3b Compare March 4, 2021 14:10
@ashcrow
Copy link
Contributor Author

ashcrow commented Mar 4, 2021

Rebased

@ashcrow
Copy link
Contributor Author

ashcrow commented Mar 4, 2021

Merging per discussion.

@ashcrow ashcrow merged commit 428e025 into keylime:master Mar 4, 2021
@lkatalin lkatalin mentioned this pull request Mar 4, 2021
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.

5 participants