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

Fixes #11389 - Remove API v1 #5068

Merged
merged 1 commit into from Jan 15, 2018
Merged

Conversation

tbrisker
Copy link
Member

API v1 has been deprecated since Foreman 1.9 - over 2 years ago.
This is more then enough time for anyone needing to migrate to v2.
Even though the original issue requests extraction to a plugin, I do not
see any point in investing the time and effort to maintain a long
deprecated API.

@theforeman-bot
Copy link
Member

Issues: #11389

Copy link
Member

@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

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

SGTM

@lzap
Copy link
Member

lzap commented Dec 11, 2017

I am all for dropping it but I remember @ohadlevy preferred putting this into plugin. But frankly, I only agree with this if there is someone committed to maintain the plugin for several releases. Anyway possible plugin is not part of this PR, so +1 for me.

@tbrisker
Copy link
Member Author

If anyone wishes to make a plugin they are more then welcome too - that doesn't mean we can't drop it from core until it exists. TBH I don't think there would be much demand for supporting this api considering how old v2 is already - and anyone still using v1 is most likely also still using an older version of Foreman.

@tbrisker
Copy link
Member Author

@xprazak2 As the release manager for 1.17 you should make the call if this should go in before or after branching.

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

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

I agree let's merge it.

@xprazak2
Copy link
Contributor

I have no objections against merging this before 1.17 branches, it does not affect packaging as far as I can see.

@tbrisker
Copy link
Member Author

@xprazak2 if we want to try and maintain semver and merge this into 1.17 i think that means 1.17 should be 2.0.

@ares
Copy link
Member

ares commented Dec 11, 2017

I think there was no clear agreement on 1.17 being 2.0. I was hoping to have a big warning that api v1 is being dropped in 1.17 release notes and merge this just after branching.

@ares
Copy link
Member

ares commented Dec 12, 2017

this needs rebasing now

API v1 has been deprecated since Foreman 1.9 - over 2 years ago.
This is more then enough time for anyone needing to migrate to v2.
Even though the original issue requests extraction to a plugin, I do not
see any point in investing the time and effort to maintain a long
deprecated API.
@tbrisker
Copy link
Member Author

@ares thanks, rebased

Copy link
Contributor

@mmoll mmoll left a comment

Choose a reason for hiding this comment

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

Let's merge this, now that 1.17 is branched.

@timogoebel
Copy link
Member

In case anybody cares to put this into a plugin, let me point you to a starter: https://github.com/unorthodoxgeek/foreman_v1

@tbrisker
Copy link
Member Author

@mmoll @timogoebel @lzap @ares - looks like there is consensus to drop it, who wants to do the honors? I added the task of indicating in 1.17 docs that it is the last version supporting apiv1 - theforeman/theforeman.org#974

@timogoebel timogoebel merged commit d4449f4 into theforeman:develop Jan 15, 2018
@timogoebel
Copy link
Member

Merged, thanks @tbrisker.

Now we have plenty of room for a new graphql api! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants