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 optional build flavor configuration #55

Merged
merged 4 commits into from
Jun 19, 2020

Conversation

clementd-fretlink
Copy link
Contributor

This requires clever-tools v1.5.0 or higher

@clementd-fretlink clementd-fretlink changed the title Add optional build flavor configuration [WIP] Add optional build flavor configuration Oct 8, 2019
@paulrbr-fl paulrbr-fl self-requested a review October 9, 2019 09:16
Copy link
Contributor

@paulrbr-fl paulrbr-fl left a comment

Choose a reason for hiding this comment

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

Very nice, the PR is easy to read 👌.

The only missing part is to add the new bash script in the list of necessary scripts for setuping the role: (in here:

with_items:
- clever-wait-deploy.sh
- clever-set-domain.sh
- clever-set-drain.sh
) you'll need to add files/clever-set-build-flavor.sh to the list)

@clementd-fretlink
Copy link
Contributor Author

ah thanks for catching that

@clementd-fretlink
Copy link
Contributor Author

I'm starting to work again on this one, now that I know how to test changes easily

@clementd-fretlink
Copy link
Contributor Author

Ok, I've tested it on lane-explorer, both with and without a value, it works as intended!

@clementd-fretlink clementd-fretlink changed the title [WIP] Add optional build flavor configuration Add optional build flavor configuration Jun 19, 2020
@clementd-fretlink
Copy link
Contributor Author

I've removed the bash script and inlined the command in the task definition

@clementd-fretlink
Copy link
Contributor Author

I have a new issue, but i don't know if it's related to my changes:

https://gitlab.in.fretlink.com/fretlink/lane-explorer/-/jobs/156256#L783

@clementd-fretlink
Copy link
Contributor Author

After some investigation, i think the error was introduced when we went from git push to clever deploy. Thankfully it does not happen in "regular" use cases (i triggered it by redeploying the same commit that was already deployed). I have an idea on how to fix it, but it's out of scope for this PR: CleverCloud/clever-tools#422

@paulrbr-fl
Copy link
Contributor

This requires clever-tools v1.5.0 or higher

can you mention it in the requirements paragraph of the README please?

README.md Outdated
@@ -35,6 +35,7 @@ Variables for the application
- _Obsolete_: `clever_metrics`: metrics used to be disabled by default. Now they are enabled by default and can be explicitly disabled with `clever_disable_metrics`.
- `clever_disable_metrics`: a boolean to disable metrics support. Optional, default to `false`.
- `clever_env_output_file`: as a post deploy task you might need to retrieve the full Clever environment configuration (i.e. with addon env variables). If this variable is set to a filename then the env will be retrieved after a successful deploy inside this file. Optional.
- `clever_build_flavor`: an optional text value used to configure the size of the dedicated build instance. `None` delegates to clever cloud default behaviour. `Some "disabled"` disables the dedicated build instance altogether.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add the variable in the defaults/main.yml file please? (for people not using Dhall)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this needed, since its default value is to be not defined? How can I do it in yaml?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed the default value in the ansible variables is not needed. However you mention two exemple values of None and Some "disabled". I think you should remove the Some part which is specific to dhall :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, good catch. I've changed the wording to avoid using dhallisms.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@clementd-fretlink clementd-fretlink merged commit af67286 into fretlink:master Jun 19, 2020
@clementd-fretlink clementd-fretlink deleted the build-flavor branch June 19, 2020 13:18
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