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

test!: Don't validate salesforce orgs against orgs.yml #15

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

feanil
Copy link
Contributor

@feanil feanil commented Sep 22, 2022

BREAKING CHANGE: We no longer test that the "Account Name" for the CSV has a valid
entry in the orgs.yaml file in the same directory. This doesn't make sense because
salesforce is now the source of truth so also needing we can trust it to have valid
orgs. And if it doesn't that should be fixed in the salesforce process and not here.

@feanil feanil requested review from e0d and nedbat September 22, 2022 15:10
Copy link
Contributor

@e0d e0d left a comment

Choose a reason for hiding this comment

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

Good change. Should we remove the file? Is there a change log?

BREAKING CHANGE: We no longer test that the "Account Name" for the CSV has a valid
entry in the orgs.yaml file in the same directory.  This doesn't make sense because
salesforce is now the source of truth so also needing we can trust it to have valid
orgs. And if it doesn't that should be fixed in the salesforce process and not here.
@feanil feanil force-pushed the feanil/update_salesforce_test branch from ac9f88d to 3e71265 Compare September 22, 2022 18:02
@feanil
Copy link
Contributor Author

feanil commented Sep 22, 2022

The org.yml file is still being used by openedx-webooks in some way so while I think it makes sense to remove it in the long term, I think the task is a bit complex so I'm not gonna do that as a part of this change.

@feanil feanil merged commit b39bd29 into master Sep 22, 2022
@feanil feanil deleted the feanil/update_salesforce_test branch September 22, 2022 18:03
@nedbat
Copy link
Contributor

nedbat commented Sep 23, 2022

Do we have a plan to put the information in salesforce and update openedx-webhooks to use it? We've removed some validation before replacing it, it seems.

@e0d
Copy link
Contributor

e0d commented Sep 23, 2022

The information is already in Salesforce, the org is included in the exported file, but we're saying we don't need an additional external validation because Salesforce is the source-of-truth. Is there a use case for another system needing the list of orgs?

@nedbat
Copy link
Contributor

nedbat commented Sep 23, 2022

Is there a use case for another system needing the list of orgs?

That would be a good thing to find out before a breaking change in the validation :)

It's perhaps not needed now, but the OSPR bot reads orgs.yaml to determine internal/external/contractor.

@e0d
Copy link
Contributor

e0d commented Sep 23, 2022

I honestly think having the CLA check file updated reliably is more important. This accomplishes that. Happy to fix forward if necessary.

@feanil
Copy link
Contributor Author

feanil commented Sep 26, 2022

@nedbat so it sounds like we need to update openedx-webhooks bot to read orgs from the CSV as well? I'll add a ticket for that.

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.

None yet

3 participants