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.

Sign up for GitHub

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

Jump to bottom

[EmbeddedAnsible] verify_ssl field on repository #7768

Merged
merged 1 commit into from Jun 30, 2021
Merged

[EmbeddedAnsible] verify_ssl field on repository #7768

merged 1 commit into from Jun 30, 2021

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Jun 24, 2021 •
edited

Requires: ManageIQ/manageiq#21298

Adds verify_ssl checkbox for the EmbeddedAnsible repository form. On the backend, this actually ends up saving the field on not the ConfigurationScriptSource, but the associated GitRepository object, but that is mostly a backend concern that is not visiable on the client side.

Screenshots

Before

Screen Shot 2021-06-24 at 4 34 47 PM

After

Screen Shot 2021-06-24 at 4 26 16 PM

TODO

Merge the following:

Steps for Testing/QA

  • Go to: Automation -> EmbeddedAnsible -> Repositories
  • Add a new repo, and ensure that Verify SSL is checked
  • Optional: If running locally, ensure that you are using simulate_queue_worker
  • Once saved, edit the new record, and ensure that verify_ssl is checked
  • Uncheck the record, and save.
  • Once saved, edit the record again to confirm the checkbox is unchecked

Much of what we want to confirm here is that the UI is properly setting and tracking the virtual_attribute :verify_ssl here. Hence the multiple save steps and checking that the UI properly reflects the state in the checkbox

NickLaMuro
NickLaMuro commented Jun 24, 2021
View reviewed changes
app/views/ansible_repository/_repository_form.html.haml Outdated
%input{:type => "checkbox",
:name => "verify_ssl",
:id => "verify_ssl",
'ng-model' => "vm.repositoryModel.verify_ssl"}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to anyone else who might not be aware...

If you use "required" => "" here, it will make it so that the checkbox is required to be checked...

This was not clear to be at the beginning, so I spent a few hours trying to figure out what else I had done wrong until I realized that modifier was the problem.

@NickLaMuro NickLaMuro force-pushed the embedded_ansible_repo_verify_ssl branch from 2b1ba53 to 332dda6 Compare June 24, 2021 21:36
@NickLaMuro NickLaMuro mentioned this pull request Jun 24, 2021
Merged
1 task
@NickLaMuro
Copy link
Member Author

@miq-bot assign @kavyanekkalapu
@miq-bot add_reviewer @Fryguy

Just an FYI: I added on to the existing form instead of trying to wait for a DDF form conversion. We can tackle that at another time, but this gets us over the hump of enabling this feature for EmbeddedAnsible repositories.

@miq-bot miq-bot requested a review from Fryguy June 24, 2021 22:14
@kavyanekkalapu
Copy link
Member

LGTM, approved.

@NickLaMuro
[EmbeddedAnsible] verify_ssl field on repository
f6dd8c3 
Adds `verify_ssl` checkbox for the `EmbeddedAnsible` repository form.
On the backend, this actually ends up saving the field on not the
`ConfigurationScriptSource`, but the associated `GitRepository` object,
but that is mostly a backend concern that is not visiable on the client
side.
@NickLaMuro NickLaMuro force-pushed the embedded_ansible_repo_verify_ssl branch from 332dda6 to f6dd8c3 Compare June 28, 2021 13:58
@miq-bot
Copy link
Member

miq-bot commented Jun 28, 2021

Checked commit NickLaMuro@f6dd8c3 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
1 file checked, 7 offenses detected

app/views/ansible_repository/_repository_form.html.haml

  • ⚠️ - Line 55 - Layout/ExtraSpacing: Unnecessary spacing detected.
  • ⚠️ - Line 55 - Layout/ExtraSpacing: Unnecessary spacing detected.
  • ⚠️ - Line 55 - Layout/ExtraSpacing: Unnecessary spacing detected.
  • ⚠️ - Line 55 - Layout/SpaceAroundOperators: Operator => should be surrounded by a single space.
  • ⚠️ - Line 55 - Layout/SpaceAroundOperators: Operator => should be surrounded by a single space.
  • ⚠️ - Line 55 - Layout/SpaceAroundOperators: Operator => should be surrounded by a single space.
  • ⚠️ - Line 55 - id attribute must be in lisp-case

@NickLaMuro
Copy link
Member Author

@Fryguy are you now good with this change?

@Fryguy Fryguy merged commit 18c9e17 into ManageIQ:master Jun 30, 2021
@Fryguy Fryguy assigned Fryguy and unassigned kavyanekkalapu Jun 30, 2021
@NickLaMuro NickLaMuro mentioned this pull request Aug 10, 2021
Closed
@Fryguy
Copy link
Member

Fryguy commented Sep 16, 2021

Backported to morphy in commit b618719.

commit b6187197a32e164a306d887ead6bb9518a3059b2
Author: Jason Frey <fryguy9@gmail.com>
Date:   Wed Jun 30 10:23:22 2021 -0400

    Merge pull request #7768 from NickLaMuro/embedded_ansible_repo_verify_ssl
    
    [EmbeddedAnsible] verify_ssl field on repository
    
    (cherry picked from commit 18c9e17a23b0a489135e13049c4203c9c45647fa)

Fryguy added a commit that referenced this pull request Sep 16, 2021
@Fryguy
Merge pull request #7768 from NickLaMuro/embedded_ansible_repo_verify…
b618719 
…_ssl

[EmbeddedAnsible] verify_ssl field on repository

(cherry picked from commit 18c9e17)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

@kavyanekkalapu kavyanekkalapu kavyanekkalapu approved these changes

Assignees

@Fryguy Fryguy

Labels
enhancement morphy/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@NickLaMuro @kavyanekkalapu @miq-bot @Fryguy @chessbyte