Skip to content

Narrow down Datatypes #2245

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

Merged
merged 1 commit into from
Jul 29, 2022
Merged

Conversation

cocker-cc
Copy link
Contributor

  • use Datatypes from Stdlib
  • use Variant[Integer, Pattern[/^[0-9]+$/]] where Strings represent Integers

@cocker-cc cocker-cc requested a review from a team as a code owner June 10, 2022 09:18
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Cocker Koch seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I am very happy with stricter data types, but this is quite hard to review due to the various things going on. I'd prefer starting with the simple things like whitespace/coding style changes. Those can be merged pretty easily.

@@ -40,8 +40,8 @@
#
define apache::balancermember (
String $balancer_cluster,
String $url = "http://${$facts['networking']['fqdn']}/",
Array $options = [],
Stdlib::HTTPUrl $url = "http://${$facts['networking']['fqdn']}/",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for context, https://httpd.apache.org/docs/2.4/mod/mod_proxy.html#balancermember is the documentation. That just mentions url, but I think HTTP/HTTPS is sufficient in schemes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as an additional Note: stdlib/types/httpurl.pp:

type Stdlib::HTTPUrl = Pattern[/(?i:\Ahttps?:\/\/.*\z)/]

So, HTTPS is included there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I was mostly thinking out loud. Should have been explicit in that.

@david22swan
Copy link
Member

@cocker-cc Look's like a lot of good changes so far.
Looking forward to getting this merged.

@cocker-cc
Copy link
Contributor Author

@cocker-cc Look's like a lot of good changes so far. Looking forward to getting this merged.

@david22swan: just wait for the Changes out of the Discussions above. Okay?

@david22swan
Copy link
Member

@cocker-cc Look's like a lot of good changes so far. Looking forward to getting this merged.

@david22swan: just wait for the Changes out of the Discussions above. Okay?

Yeh, that's what I meant sorry. Should have been clearer.

@david22swan
Copy link
Member

@cocker-cc Hey, just checking if there are any updates on this coming soon?

@ekohl
Copy link
Collaborator

ekohl commented Jul 4, 2022

It may be good to wait for #2255 before rebasing this.

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Please rebase this now that the related PRs have been merged.

Also, as I suggested inline, feel free to narrow things down if they make more sense. It's going to be a major version bump, let's make use of it.

@cocker-cc cocker-cc force-pushed the Narrow_down_Datatypes branch 2 times, most recently from ef41982 to 55e7bc7 Compare July 10, 2022 09:20
@cocker-cc cocker-cc force-pushed the Narrow_down_Datatypes branch from 55e7bc7 to 0c28a88 Compare July 11, 2022 18:04
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Once CI passes I'd be happy to merge this and iterate on it further if we find breakages.

@cocker-cc cocker-cc force-pushed the Narrow_down_Datatypes branch from 0c28a88 to bdf1020 Compare July 18, 2022 08:39
ekohl
ekohl previously approved these changes Jul 20, 2022
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'm going to merge this. We'll probably see some fallout from this given its size, but it's moving things forward a lot.

@ekohl
Copy link
Collaborator

ekohl commented Jul 20, 2022

Oh, I missed that there is now a merge conflict in spec/acceptance/mod_php_spec.rb. Once you fix that, I'll merge this soon to avoid further conflicts.

@cocker-cc
Copy link
Contributor Author

Oh, I missed that there is now a merge conflict in spec/acceptance/mod_php_spec.rb.

done

@cocker-cc cocker-cc force-pushed the Narrow_down_Datatypes branch from 280ec5f to bfa81ab Compare July 20, 2022 14:13
ekohl
ekohl previously approved these changes Jul 20, 2022
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Let's see if CI is green.

@david22swan
Copy link
Member

Look's like a few type mismatches in the test's:

Error while evaluating a Resource Statement, Class[Apache]: parameter 'service_ensure' expects a match for Stdlib::Ensure::Service = Enum['running', 'stopped'], got Boolean
Error while evaluating a Resource Statement, Class[Apache]: parameter 'server_signature' expects a match for Variant[Enum['Off', 'On'], Stdlib::Email = Pattern[/\A[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*\z/]], got 'testsig' 
etc.

@cocker-cc
Copy link
Contributor Author

Error while evaluating a Resource Statement

I updated some Puppet-Datatypes.

@cocker-cc cocker-cc force-pushed the Narrow_down_Datatypes branch 2 times, most recently from 235894e to 93e7d0d Compare July 26, 2022 11:15
@cocker-cc
Copy link
Contributor Author

Again I reverted some Datatypes back to String.

@cocker-cc cocker-cc force-pushed the Narrow_down_Datatypes branch 2 times, most recently from 8489efe to 5b2d902 Compare July 26, 2022 13:14
@cocker-cc
Copy link
Contributor Author

cocker-cc commented Jul 27, 2022

2022-07-27T14:20:28.5731650Z        expected that the catalogue would contain File[30-rspec.example.com.conf symlink] with path set to "/etc/apache2/sites-enabled/30-rspec.example.com.conf" but it is set to "/etc/httpd/sites-enabled/30-rspec.example.com.conf"

Have no Idea, why these appear. In the Specfile it's set to apache2, and in the PP-File it's set to apache2.

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Those also happen in main. I'll take a quick look.

@@ -1,3 +1,4 @@
# @summary disable Apache-Module event
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps this and the other disable classes should also have @api private

@ekohl
Copy link
Collaborator

ekohl commented Jul 28, 2022

I believe #2266 should fix it.

- use Datatypes from Stdlib, particularly a Port now only accepts Stdlib::Port
- use Variant[Integer, Pattern[/^[0-9]+$/]] where Strings represent Integers
@cocker-cc cocker-cc force-pushed the Narrow_down_Datatypes branch from 5b2d902 to f41251e Compare July 28, 2022 11:00
@ekohl
Copy link
Collaborator

ekohl commented Jul 28, 2022

Looks like the tests will be green (Puppet 7 already passed). The only thing appears to be the CLA.

Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

It's been a long road but LGTM
Thank you @cocker-cc for putting in the work and @ekohl for all the reviews.

Won't merge immediately as with the size of this I'd like some others on the team to give it a look over first, but all being good should be merged in by Monday evening at the latest, with it and all the other changes released by that Friday or the next Monday.

ldap_cache_ttl: '600',
ldap_opcache_entries: '1024',
ldap_opcache_ttl: '600',
ldap_shared_cache_size: 500_000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the underscore a typo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, in Ruby this is a convention to make numbers easier to read. Because it's quite hard to see if it's 5 or 6 zeros, this makes it visually easier to see it's 500k, not 5m. Rubocop even enforces it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing, never knew this 👍 will definitely come in handy at some point :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmcmaw: Sharp Eyes 👀
I do not like it, but Rubocop with its Config-File coming from the PDK-Templates sais so.

@pmcmaw
Copy link
Contributor

pmcmaw commented Jul 29, 2022

I have just posted one of the tiniest questions, other than that it LGTM.

@david22swan
Copy link
Member

david22swan commented Jul 29, 2022

@cocker-cc Could you sign the CLA?
Once that is done I can hit merge

@cocker-cc
Copy link
Contributor Author

@cocker-cc Could you sign the CLA? Once that is done I can hit merge

I did, but it does not show up.

Screenshot from 2022-07-29 12-12-43

@david22swan
Copy link
Member

Yeh sorry bout that, it can happen sometimes.
Screenshot's good enough for me, will go ahead and merge.
Thank's again to the both of you for putting in all this work :)

@david22swan david22swan merged commit 950cffb into puppetlabs:main Jul 29, 2022
@pmcmaw
Copy link
Contributor

pmcmaw commented Jul 29, 2022

@cocker-cc Amazing PR, thank you for your time and effort! 🥳

@cocker-cc cocker-cc deleted the Narrow_down_Datatypes branch July 29, 2022 15:21
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.

6 participants