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

Fix small bugs and add pre- and post-hooks to some roles #175

Merged
merged 1 commit into from
Jan 6, 2021

Conversation

wheelerlaw
Copy link
Contributor

@wheelerlaw wheelerlaw commented Dec 10, 2020

I just noticed this in the output:

TASK [cleanup : Is project repository present in /home/wlaw/.contra-env-setup/pipeline] ************************************************************************************************************************************************
ok: [localhost]

TASK [cleanup : Cleanup /home/wlaw/.contra-env-setup/pipeline] *************************************************************************************************************************************************************************
skipping: [localhost] => (item=/home/wlaw/.contra-env-setup//home/wlaw/.contra-env-setup/pipeline)

You can see in the Cleanup /home/wlaw/.contra-env-setup/pipeline task, it is prepending the path with another path, making the path invalid, so it is skipped.

Also as a part of this MR, the behavior around start_minishift was made more consistent, since how that variable was used depended on whether minishift was being provisioned locally or on a remote VM. Now it is more consistent, in that the start_minishift variable is respected regardless if the VM is local or remote. The default value for start_minishift was changed to true, since it is being started anyway by the os_temps playbook.

Finally, pre- and post-hooks were added to some of the playbooks to support adding in custom behavior by any consumers of this project.

@cp-paas-bot
Copy link

Can one of the admins verify this patch?

@wheelerlaw
Copy link
Contributor Author

Okay, I also cleaned up some inconsistencies with the startup logic for minishift, and I added some pre- and post-hooks for some of the roles. That way, I can inject the CA cert into minishift when its done being setup but before CVP is deployed.

@dirgim dirgim self-requested a review December 10, 2020 09:00
Copy link
Collaborator

@dirgim dirgim left a comment

Choose a reason for hiding this comment

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

The changes look pretty good apart from a couple of nits - and since the PR has grown to include more structural changes (additional hooks), I'd ask for the commit and PR titles to be changed to reflect that.

@wheelerlaw wheelerlaw changed the title Fix small bug in cleanup role Fix small bugs and add pre- and post-hooks to some roles Dec 10, 2020
@wheelerlaw
Copy link
Contributor Author

I updated the PR with the requested changes, and also updated the title and description of the PR to reflect the scope of the changes.

@wheelerlaw
Copy link
Contributor Author

Reworded commit message, and squashed all commits into one.

…e startup logic, and added some pre- and post-hooks to some of the roles.
@wheelerlaw
Copy link
Contributor Author

Added some more tweaks, and squashed.

@wheelerlaw
Copy link
Contributor Author

@dirgim Bump

Copy link
Collaborator

@dirgim dirgim left a comment

Choose a reason for hiding this comment

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

LGTM

@dirgim dirgim merged commit b85609f into CentOS-PaaS-SIG:master Jan 6, 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.

None yet

3 participants