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

Remove 3.8 repo, use pl-apt 4.4 https support, clean coding #170

Merged
merged 3 commits into from
Dec 28, 2018

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jul 13, 2018

No description provided.

@bastelfreak
Copy link
Member

Hi @ekohl, can you rebase please?

@ekohl
Copy link
Member Author

ekohl commented Aug 4, 2018

Rebased

@ekohl
Copy link
Member Author

ekohl commented Nov 26, 2018

Rebased again

@bastelfreak
Copy link
Member

Notice: /Stage[main]/Apt::Update/Exec[apt_update]/returns: E: The method driver /usr/lib/apt/methods/https could not be found.

Why do I have a feeling that https for apt is broken every two weeks? :(

@ekohl
Copy link
Member Author

ekohl commented Nov 26, 2018

Could be. Wonder if there's some chaining missing.

@dhoppe
Copy link
Member

dhoppe commented Dec 3, 2018

@ekohl I think you should not remove the package apt-transport-https and fix the value of repo_url, because the web server makes an redirect vom http to https.

@ekohl
Copy link
Member Author

ekohl commented Dec 3, 2018

@dhoppe
Copy link
Member

dhoppe commented Dec 3, 2018

@ekohl Cool. I must have been away too long, but now you need to fix the protocol for the tests aswell. But I am still not sure about this change #184.

@ekohl
Copy link
Member Author

ekohl commented Dec 3, 2018

I did copy that part yes because I think it's more correct. I can leave it out for now and merge it in a separate PR.

@ekohl
Copy link
Member Author

ekohl commented Dec 3, 2018

Updated. The http -> https URL change should also fix the failing puppetlabs-apt tests. That does a regex on the URL to decide.


# basic sanity check
if $version == 'LATEST' {
$repo_ver = $version
} elsif $version =~ /^\d\.\d+$/ {
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to convert this into a datatype?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not that much because AFAIK you can't really extract variables from a data type like we do in the case below. You could avoid the else case at the end, but an if/elsif/elsif tree without an else feels wrong to me.

@bastelfreak
Copy link
Member

@ekohl can you please rebase and give me a hint afterwards? I would like to review and merge this as soon as possible.

@ekohl
Copy link
Member Author

ekohl commented Dec 28, 2018

Rebased

@bastelfreak
Copy link
Member

can you take a look at the failing spec tests?

@ekohl
Copy link
Member Author

ekohl commented Dec 28, 2018

Updated

@bastelfreak
Copy link
Member

thanks!

@bastelfreak bastelfreak merged commit 363405b into voxpupuli:master Dec 28, 2018
@ekohl ekohl deleted the cleanup-repo branch December 29, 2018 13:01
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