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 bugs for role validate servers #1319

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

ShPakvel
Copy link
Contributor

@ShPakvel ShPakvel commented Dec 22, 2024

There is syntactic typo-bug during validate_servers! invocation for role (wrong number of arguments).
It wasn't discovered because it has never met condition to run on. Role config wasn't correctly extracted for validation - another logical bug.
Both bugs fixed.

Also remove not used anymore accessories_on. Leftover from previous changes.

There were typo-bug during `validate_servers!` invocation for role.
It wasn't discovered, because it never met condition. Because role_config wasn't correctly extracted for validation.

Also remove not used anymore `accessories_on`. Leftover from previous changes.
@ShPakvel ShPakvel changed the title Fix bug in role validate_servers. Fix bugs in role validate_servers Dec 22, 2024
@ShPakvel ShPakvel changed the title Fix bugs in role validate_servers Fix bugs for role validate servers Dec 22, 2024
@djmb
Copy link
Collaborator

djmb commented Jan 17, 2025

Hi @ShPakvel - is it possible to write a test for this condition?

@ShPakvel
Copy link
Contributor Author

Hi @ShPakvel - is it possible to write a test for this condition?

Regarding to fix for syntax bug with wrong number of arguments

Existing tests already cover it. Here is example of test execution.
As I wrote, it wasn't caught by tests because of another logical bug interfered and prevented flow to validation.

$ ./bin/test test/configuration/role_test.rb

# ...

Error:
ConfigurationRoleTest#test_cmd:
ArgumentError: wrong number of arguments (given 2, expected 1)
    lib/kamal/configuration/validator.rb:110:in `validate_servers!'
    lib/kamal/configuration/validator/role.rb:6:in `validate!'
    lib/kamal/configuration/validation.rb:21:in `validate!'
    lib/kamal/configuration/role.rb:12:in `initialize'

# ...

bin/test /home/shpakvel/projects/github/kamal/test/configuration/role_test.rb:32

Finished in 0.069962s, 285.8696 runs/s, 57.1739 assertions/s.
20 runs, 4 assertions, 0 failures, 18 errors, 0 skips

And tests are passed when code is corrected.

$ ./bin/test test/configuration/role_test.rb
Run options: --seed 49549

# Running:

....................

Finished in 0.190348s, 105.0708 runs/s, 262.6770 assertions/s.
20 runs, 50 assertions, 0 failures, 0 errors, 0 skips

Regarding logical bug.

  • It looks like simple overlooked difference between specializations and role_config. And after this change, difference is clearly stated in code logic.
  • And right now it is private part of class.

Which leads us to choose between these ways to proceed:

  • A - to consider this part as internal (private) that shouldn't be tested.
  • B - to make specializations and role_config methods public, and right tests on what they should return.

@djmb, Let me know which way you prefer A or B. If it is B, I will proceed with changes and tests creation.

@djmb
Copy link
Collaborator

djmb commented Jan 17, 2025

Ok, that makes sense, thanks! All good to merge as it is then

@djmb djmb merged commit eee9d67 into basecamp:main Jan 17, 2025
8 checks passed
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