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

Delete obsolete CheckEnforcer tool #4963

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

konrad-jamrozik
Copy link
Contributor

@konrad-jamrozik konrad-jamrozik commented Dec 14, 2022

Deleting obsolete, no longer used sources of check enforcer, per this comment: #4937 (review). Now we use a different version of it, based on GitHub actions.

Related work item:

Once this PR is merged, I will remove the internals / tool - check-enforcer pipeline as well.

@weshaggard @benbp I believe the README in the removed directory (tools/check-enforcer/README) probably has bunch of useful information and should be reclaimed/moved. I also want to use this as an opportunity to review and update our check enforcer docs. Hence my questions:

  • Where should I move the README? To doc directory, and link to it from top-level README?
  • Where is the currently active check enforcer source? Where is its documentation?

Thanks!

@konrad-jamrozik konrad-jamrozik changed the title Delete obsolete CheckEnforcer sources Delete obsolete CheckEnforcer source Dec 14, 2022
@benbp
Copy link
Member

benbp commented Dec 14, 2022

FYI there is still one more repository using the old check enforcer, but I think it's ok to start deleting the code while we keep the service running. I have an update to migrate this repository here, but was blocked at the time on a minor issue. I will follow-up on this.

@konrad-jamrozik it is ok to delete the readme, the docs have been moved/updated and live here. The active check-enforcer source is in that same repo here.

@konrad-jamrozik konrad-jamrozik changed the title Delete obsolete CheckEnforcer source Delete obsolete CheckEnforcer tool Dec 14, 2022
@konrad-jamrozik konrad-jamrozik requested a review from a team as a code owner December 14, 2022 21:42
@konrad-jamrozik konrad-jamrozik force-pushed the users/kojamroz/iss_4888_remove_check_enforcer branch from b721955 to 7b6d506 Compare December 14, 2022 21:51
@konrad-jamrozik
Copy link
Contributor Author

konrad-jamrozik commented Dec 14, 2022

FYI there is still one more repository using the old check enforcer, but I think it's ok to start deleting the code while we keep the service running. I have an update to migrate this repository here, but was blocked at the time on a minor issue. I will follow-up on this.

@konrad-jamrozik it is ok to delete the readme, the docs have been moved/updated and live here. The active check-enforcer source is in that same repo here.

@benbp for the source link, you linked just to the repo, but I guess you meant perhaps to link to the main.go file?

Also, please note that since you approved this PR I have updated the main README to point to the new Check Enforcer location, and I have removed the CHECKENFORCER file.

I will keep this PR open until we are fully migrated and we can decommission the tool. Just to avoid potential confusion on what is going on.

@@ -1,41 +0,0 @@
format: v0.1-alpha
Copy link
Member

Choose a reason for hiding this comment

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

@benbp did you have any plans to use this configuration with the new checkenforcer? If not then we should probably remove this from the other repos too.

Copy link
Member

Choose a reason for hiding this comment

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

No plans. I have an item on my todo to remove the other markdown files from the repos.

Copy link
Member

Choose a reason for hiding this comment

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

Ok make sure your todo list translates to github issues so we don't lose them.

Copy link
Contributor Author

@konrad-jamrozik konrad-jamrozik Dec 15, 2022

Choose a reason for hiding this comment

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

@benbp I can go ahead and get rid of all the CHECKENFORCER files, given I already work on this.

Can I maybe also somehow assist with merging this?
Azure/azure-dev#981

Is this about figuring out better name for the event.yml file?

Just hoping here to wrap-up this removal work and not leave anything dangling.

Copy link
Contributor Author

@konrad-jamrozik konrad-jamrozik Dec 20, 2022

Choose a reason for hiding this comment

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

@benbp I can go ahead and get rid of all the CHECKENFORCER files, given I already work on this.

Can I maybe also somehow assist with merging this? Azure/azure-dev#981

Is this about figuring out better name for the event.yml file?

Just hoping here to wrap-up this removal work and not leave anything dangling.

Postponed until:

@konrad-jamrozik konrad-jamrozik self-assigned this Dec 15, 2022
@konrad-jamrozik konrad-jamrozik added the Central-EngSys This issue is owned by the Engineering System team. label Dec 15, 2022
@konrad-jamrozik
Copy link
Contributor Author

We are having discussion about this PR on internal Teams chat on Engineering System / Azure SDK, here.

Looks it the new GitHub-action based Check Enforcer possibly cannot be triggered by other GitHub workflows, which makes it unsuitable for https://github.com/Azure/azure-dev, as it uses such workflows. We are currently discussing possible approaches.

@konrad-jamrozik
Copy link
Contributor Author

I am closing this PR for now, as there is more work to be done before we can decommission the tool. I captured our findings and work required here:

Once the prerequisite work is done, we can re-open this PR.

@konrad-jamrozik konrad-jamrozik deleted the users/kojamroz/iss_4888_remove_check_enforcer branch January 5, 2023 05:43
@konrad-jamrozik konrad-jamrozik restored the users/kojamroz/iss_4888_remove_check_enforcer branch January 26, 2023 06:39
@konrad-jamrozik
Copy link
Contributor Author

konrad-jamrozik commented Jan 26, 2023

@benbp I have deleted the resource groups used by the legacy CheckEnforcer, per this comment. I believe now we can delete the tool source itself, hence reopening this PR. Note that the deleted azure-sdk-tools\tools\check-enforcer\ci.yml was referencing the deleted resource groups.

@konrad-jamrozik konrad-jamrozik force-pushed the users/kojamroz/iss_4888_remove_check_enforcer branch from 9c2c2f3 to 9376f64 Compare January 26, 2023 06:43
- Update README for Check Enforcer
- Delete obsolete CHECKENFORCER file
@konrad-jamrozik konrad-jamrozik force-pushed the users/kojamroz/iss_4888_remove_check_enforcer branch from 9376f64 to 4f6541e Compare January 26, 2023 06:44
@konrad-jamrozik
Copy link
Contributor Author

/check-enforcer override

@konrad-jamrozik konrad-jamrozik merged commit 32847c3 into main Jan 26, 2023
@konrad-jamrozik konrad-jamrozik deleted the users/kojamroz/iss_4888_remove_check_enforcer branch January 26, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants