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: check if login exists in grafana organization user exists check #310

Conversation

rustyb
Copy link

@rustyb rustyb commented Aug 14, 2023

SUMMARY

Nice collection much appreciated.

I noticed a small bug in the organisation user exists check when using v1.6.0.

When you add a user to an organisation you provide the login name. Once the user is created in the organisation and you run the task again you always get the error message Exception: [BUG] User not found in organization

I had not defined names on the users I created so the check was not finding them as the name field of each user was ''.

This brought me to _organization_user_by_login which is only checking the name and email fields.

I've updated the check to also check login against the login field of the user.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

Issue with grafana_organization_user -> _organization_user_by_login.

ADDITIONAL INFORMATION

I had an existing grafana initalised with several users and was attempting to add new organisations and place users within them. I used the following tasks list. First run goes well. Subsequent runs fail at "Add user to the organization" due to the name field being empty on my users.

---
    - name: Create a Grafana organization
      community.grafana.grafana_organization:
        url: "{{ grafana_url }}"
        url_username: "{{ grafana_security.admin_user }}"
        url_password: "{{ grafana_security.admin_password  }}"
        name: cbr test
        state: present 
      register: org
   
    - name: Add user to the organization
      community.grafana.grafana_organization_user:
        url: "{{ grafana_url }}"
        url_username: "{{ grafana_security.admin_user }}"
        url_password: "{{ grafana_security.admin_password  }}"
        org_id: "{{ org.org.id }}"
        login: "cbr"
        role: admin
        state: present

@rustyb rustyb requested review from rrey and seuf as code owners August 14, 2023 19:12
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0ae8c31) 71.03% compared to head (971e1c4) 70.04%.

❗ Current head 971e1c4 differs from pull request most recent head 5060bca. Consider uploading reports for the commit 5060bca to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #310      +/-   ##
==========================================
- Coverage   71.03%   70.04%   -1.00%     
==========================================
  Files          18        9       -9     
  Lines        1854     1155     -699     
  Branches      319      254      -65     
==========================================
- Hits         1317      809     -508     
+ Misses        396      200     -196     
- Partials      141      146       +5     
Flag Coverage Δ
integration 69.87% <100.00%> (+0.12%) ⬆️
sanity ?
units ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
plugins/modules/grafana_organization_user.py 85.89% <100.00%> (-1.29%) ⬇️

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Nemental
Copy link
Collaborator

Nemental commented Nov 6, 2023

Hi! I just stumbled over your issue and noticed that I ran into the same problem as you in my PR (#318). Unlike in your solution, I didn't include the “name” attribute in the query because, according to the API, it is not included in the login check.
https://grafana.com/docs/grafana/latest/developers/http_api/org/#add-user-in-organization
loginOrEmail - The user's name field is not used when logging in.

@rustyb
Copy link
Author

rustyb commented Nov 8, 2023

@Nemental had completely forgot about this one. Yes indeed it seems like the solution in your PR would also work for my use case too.

@rrey
Copy link
Collaborator

rrey commented Nov 8, 2023

@rustyb can you chack that your issue is fixed now that @Nemental's patch is merged ? 🙏

@rustyb
Copy link
Author

rustyb commented Nov 11, 2023

@rrey I can confirm that #318 solves my issue - feel free to close this PR 👍

@rrey
Copy link
Collaborator

rrey commented Nov 11, 2023

Awesome !

@rrey rrey closed this Nov 11, 2023
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.

3 participants