Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Added support for removing images/tags #854

Merged
merged 4 commits into from
May 9, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
inherit_from:
- ./config/rubocop-suse.yml

# TODO: (mssola) only the LDAP class requires this.
# TODO: (mssola) only the LDAP class and portusctl require this.
Metrics/ClassLength:
Max: 150
Max: 160

# TODO: (mssola) Some methods are offending this cop. In the SUSE's style guide
# the approach is to use Rubocop's default value. In the near future I will
Expand Down
8 changes: 8 additions & 0 deletions app/assets/stylesheets/repositories.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
.tags .label.label-success {
margin: 0px 2px;
}

#remove-repo button {
padding: 0px 10px 0px 0px;
}

.remove-repo:focus, .remove-repo:hover, .remove-tag:focus, .remove-tag:hover {
text-decoration: none;
}
14 changes: 14 additions & 0 deletions app/controllers/concerns/delete_enabled.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# DeleteEnabeld redirects the user back if delete support is not enabled. A
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo, should read DeleteEnabled.

# `before_action` will be created for the :destroy method.
module DeleteEnabled
Copy link
Contributor

Choose a reason for hiding this comment

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

There is some convention to name concerns like polymorph activerecord relations ending with able, e.g. Deletable

extend ActiveSupport::Concern

included do
before_action :delete_enabled?, only: [:destroy]
end

# Returns true if users can delete images/tags.
def delete_enabled?
redirect_to :back, status: :forbidden unless APP_CONFIG.enabled?("delete")
end
end
26 changes: 25 additions & 1 deletion app/controllers/repositories_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
class RepositoriesController < ApplicationController
before_action :set_repository, only: [:show, :toggle_star]
include DeleteEnabled

before_action :set_repository, only: [:show, :destroy, :toggle_star]

# GET /repositories
# GET /repositories.json
Expand All @@ -23,6 +25,28 @@ def toggle_star
render template: "repositories/star", locals: { user: current_user }
end

# Removes all the tags that belong to this repository, and removes it.
def destroy
# First of all we mark the repo and the tags, so we don't have concurrency
# problems when "delete" events come in.
@repository.tags.update_all(marked: true)
@repository.update_attributes(marked: true)

# Remove all tags, effectively removing them from the registry too.
@repository.groupped_tags.map { |t| t.first.delete_by_digest!(current_user) }

# Delete this repository if all tags were successfully deleted.
if @repository.reload.tags.any?
redirect_to repository_path(@repository), alert: "Could not remove all the tags"
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be useful to dump more information into Rails' log (like which tags could not have been deleted and eventually a reason). Otherwise an admin will have a hard time figuring this out on his own.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is logged, because it ends up calling Tag#delete_by_digest!, which logs any errors. Maybe I can provide an extra line of: "Error: tags [...] could not be removed" (the reason on why they have been removed is logged by Tag#delete_by_digest!)

else
@repository.delete_and_update!(current_user)
redirect_to namespace_path(@repository.namespace),
notice: "Repository removed with all its tags"
end
end

protected

def set_repository
@repository = Repository.find(params[:id])
end
Expand Down
24 changes: 24 additions & 0 deletions app/controllers/tags_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
class TagsController < ApplicationController
include DeleteEnabled

# Removes all tags the match the digest of the tag with the given ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

... that match ...

# Moreover, it will also remove the image if it's left empty after removing
# the tags.
def destroy
tag = Tag.find(params[:id])

# And now remove the tag by the digest. If the repository containing said
# tags becomes empty after that, remove it too.
repo = tag.repository
if tag.delete_by_digest!(current_user)
if repo.tags.empty?
repo.delete_and_update!(current_user)
redirect_to namespace_path(repo.namespace), notice: "Image removed with all its tags"
else
redirect_to repository_path(tag.repository), notice: "Tag removed successfully"
end
else
redirect_to repository_path(tag.repository), alert: "Tag could not be removed"
Copy link
Member

Choose a reason for hiding this comment

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

Is something logged also in this case, can the admin get a clue about why this failed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tag#delete_by_digest! already logs any error 😉

end
end
end
50 changes: 37 additions & 13 deletions app/helpers/repositories_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,28 @@
# them, dangling repositories that used to contain them. Because of this, this
# helper renders the proper HTML for push activities, while being safe at it.
module RepositoriesHelper
# Renders a push activity, that is, a repository has been pushed.
# Renders a push activity, that is, a repository/tag has been pushed.
def render_push_activity(activity)
owner = content_tag(:strong, "#{fetch_owner(activity)} pushed ")
render_repo_activity(activity, "pushed")
end

# Renders a delete activity, that is, a repository/tag has been deleted.
def render_delete_activity(activity)
render_repo_activity(activity, "deleted")
end

protected

# General method for rendering an activity regarding repositories.
def render_repo_activity(activity, action)
owner = content_tag(:strong, "#{fetch_owner(activity)} #{action} ")

namespace = render_namespace(activity)
namespace += " / " unless namespace.empty?

owner + namespace + render_repository(activity)
end

protected

# Fetches the owner of the activity in a safe way.
def fetch_owner(activity)
activity.owner.nil? ? "Someone" : activity.owner.username
Expand All @@ -27,22 +37,24 @@ def fetch_owner(activity)
def render_namespace(activity)
tr = activity.trackable

if tr.nil?
if tr.nil? || tr.is_a?(Namespace)
if activity.parameters[:namespace_name].nil?
""
else
namespace = Namespace.find_by(id: activity.parameters[:namespace_id])
if namespace.nil?
content_tag(:span, activity.parameters[:namespace_name])
else
link_to activity.parameters[:namespace_name], namespace
end
tag_or_link(namespace, activity.parameters[:namespace_name])
end
else
link_to tr.namespace.clean_name, tr.namespace
end
end

# Returns a link if the namespace is not nil, otherwise just a tag with the
# given name.
def tag_or_link(namespace, name)
namespace.nil? ? content_tag(:span, name) : link_to(name, namespace)
end

# Renders the repository part of the activity in a safe manner.
def render_repository(activity)
repo, link, tag = get_repo_link_tag(activity)
Expand All @@ -59,16 +71,17 @@ def get_repo_link_tag(activity)
tr = activity.trackable

if tr.nil?
if activity.parameters[:repo_name].nil?
if repo_name(activity).nil?
["a repository", nil, ""]
else
repo = activity.parameters[:repo_name]
repo = repo_name(activity)
ns = Namespace.find_by(id: activity.parameters[:namespace_id])
link = ns.nil? ? nil : namespace_path(ns.id)
[repo, link, tag_part(activity)]
end
else
[tr.name, tr, tag_part(activity)]
name, l = name_and_link(tr, activity)
[name, l, tag_part(activity)]
end
end

Expand All @@ -86,4 +99,15 @@ def tag_part(activity)
":#{activity.recipient.name}"
end
end

# Fetch the name of the repo from the given activity.
def repo_name(activity)
activity.parameters[:repo_name] || activity.parameters[:repository_name]
end

# Returns the name and the link to the given tr depending on whether it's a
# Namespace or not.
def name_and_link(tr, activity)
tr.is_a?(Namespace) ? [repo_name(activity), nil] : [tr.name, tr]
end
end
5 changes: 3 additions & 2 deletions app/jobs/catalog_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ def update_registry!(catalog)

# At this point, the remaining items in the "repos" array are repos that
# exist in the DB but not in the catalog. Remove all of them.
Tag.where(repository_id: dangling_repos).find_each(&:delete_and_update!)
Repository.where(id: dangling_repos).destroy_all
portus = User.find_by(username: "portus")
Tag.where(repository_id: dangling_repos).find_each { |t| t.delete_and_update!(portus) }
Repository.where(id: dangling_repos).find_each { |r| r.delete_and_update!(portus) }
end
end
3 changes: 1 addition & 2 deletions app/models/namespace.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@
# updated_at :datetime not null
# team_id :integer
# public :boolean default("0")
# registry_id :integer not null
# registry_id :integer
# global :boolean default("0")
# description :text(65535)
#
Copy link
Member

Choose a reason for hiding this comment

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

Did we really get rid of this field on the db?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Leftover...

# Indexes
#
Expand Down
40 changes: 36 additions & 4 deletions app/models/repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
# namespace_id :integer
# created_at :datetime not null
# updated_at :datetime not null
# marked :boolean default("0")
#
# Indexes
#
Expand Down Expand Up @@ -59,6 +60,33 @@ def groupped_tags
end
end

# Updates the activities related to this repository and adds a new activity
# regarding the removal of this.
def delete_and_update!(actor)
logger.tagged("catalog") { logger.info "Removed the image '#{name}'." }

# Take care of current activities.
PublicActivity::Activity.where(trackable: self).update_all(
trackable_type: Namespace,
trackable_id: namespace.id,
recipient_type: nil
)

# Add a "delete" activity"
namespace.create_activity(
:delete,
owner: actor,
recipient: self,
parameters: {
repository_name: name,
namespace_id: namespace.id,
namespace_name: namespace.clean_name
}
)

destroy
end

# Handle a push event from the registry.
def self.handle_push_event(event)
registry = Registry.find_from_event(event)
Expand All @@ -82,11 +110,15 @@ def self.handle_delete_event(event)
# Fetch the repo.
ns, repo_name, = registry.get_namespace_from_event(event, false)
repo = ns.repositories.find_by(name: repo_name)
return if repo.nil?
return if repo.nil? || repo.marked?

# Destroy tags and the repository if it's empty now.
repo.tags.where(digest: event["target"]["digest"]).map(&:delete_and_update!)
repo.destroy if !repo.nil? && repo.tags.empty?
user = User.find_from_event(event)
repo.tags.where(digest: event["target"]["digest"], marked: false).map do |t|
t.delete_and_update!(user)
end
repo = repo.reload
repo.delete_and_update!(user) if !repo.nil? && repo.tags.empty?
end

# Add the repository with the given `repo` name and the given `tag`. The
Expand Down Expand Up @@ -169,7 +201,7 @@ def self.create_or_update!(repo)
end

# Finally remove the tags that are left and return the repo.
repository.tags.where(name: to_be_deleted_tags).find_each(&:delete_and_update!)
repository.tags.where(name: to_be_deleted_tags).find_each { |t| t.delete_and_update!(portus) }
Copy link
Member

Choose a reason for hiding this comment

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

This will mark the deletion event as to be generated by the portus user. However into your screenshot the event is assigned to the right user.

How is that happening?

Copy link
Collaborator Author

@mssola mssola May 6, 2016

Choose a reason for hiding this comment

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

This is because there are two paths when it comes to deleting a repo/tag:

  1. A delete event has come.
  2. The user actually clicks on the browser.

On the second case, we remove images on the registry, and if we are successful we remove them from the DB. The problem with that is that by "remove images on the registry" I mean that there will be a delete notification for this same repository/tag 😄. This is why I've added a marked attribute: on the second case we "mark" tags that are to be deleted before calling the registry to delete the tags, as a way to lock the delete event from removing the tags that we want to remove. We do this because in this case we know who performed the action. In the case of the delete notification, no actor is given, so we don't know who effectively removed the tag.

This line of code you're pointing at refers to the first case: a delete notification. Since we don't know who performed the action, we set "portus" there. It's a similar situation as when crono creates repositories in the DB when Portus was not properly sync'ed by the web notification.

repository.reload
end
end
82 changes: 80 additions & 2 deletions app/models/tag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
# user_id :integer
# digest :string(255)
# image_id :string(255) default("")
# marked :boolean default("0")
#
# Indexes
#
Expand All @@ -29,9 +30,74 @@ class Tag < ActiveRecord::Base
# and that's guaranteed to have a good format.
validates :name, uniqueness: { scope: "repository_id" }

# Delete all the tags that match the given digest. Call this method if you
# want to:
#
# - Safely remove tags (with its re-tags) on the DB.
# - Remove the manifest digest on the registry.
# - Preserve the activities related to the tags that are to be removed.
#
# Returns true on success, false otherwise.
def delete_by_digest!(actor)
dig = fetch_digest
return false if dig.blank?

Tag.where(digest: dig).update_all(marked: true)

begin
Registry.get.client.delete(repository.name, dig, "manifests")
rescue StandardError => e
Rails.logger.error "Could not delete tag on the registry: #{e.message}"
return false
end

Tag.where(digest: dig).map { |t| t.delete_and_update!(actor) }
end

# Delete this tag and update its activity.
def delete_and_update!
def delete_and_update!(actor)
logger.tagged("catalog") { logger.info "Removed the tag '#{name}'." }

# If the tag is no longer there, ignore this call and return early.
unless Tag.find_by(id: id)
logger.tagged("catalog") { logger.info "Ignoring..." }
return
end

# Delete tag and create the corresponding activities.
destroy
create_delete_activities!(actor)
end

protected

# Fetch the digest for this tag. Usually the digest should already be
# initialized since it's provided by the event notification that created this
# tag. However, it might happen that the digest column is left blank (e.g.
# legacy Portus, unknown error, etc). In these cases, this method will fetch
# the manifest from the registry.
#
# Returns a string containing the digest on success. Otherwise it returns
# nil.
def fetch_digest
if digest.blank?
client = Registry.get.client

begin
_, dig, = client.manifest(repository.name, name)
update_attributes(digest: dig)
dig
rescue StandardError => e
Rails.logger.error "Could not fetch manifest digest: #{e.message}"
nil
end
else
digest
end
end

# Create/update the activities for a delete operation.
def create_delete_activities!(actor)
PublicActivity::Activity.where(recipient: self).update_all(
parameters: {
namespace_id: repository.namespace.id,
Expand All @@ -40,6 +106,18 @@ def delete_and_update!
tag_name: name
}
)
destroy

# Create the delete activity.
repository.create_activity(
:delete,
owner: actor,
recipient: self,
Copy link
Member

Choose a reason for hiding this comment

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

Here self is the instance of Tag that has just been deleted. This should break the final render, isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really, because the final render will pick up the stuff stored in the parameters. The thing here is that we rely on the parameters when stuff is deleted.

parameters: {
repository_name: repository.name,
namespace_id: repository.namespace.id,
Copy link
Member

Choose a reason for hiding this comment

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

This can change in the future because I expect users will request to be able to delete also a namespace and all their contents. Deleting a namespace would cause this activity to break.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, whenever we propose removing namespaces, we would have to work around it in the same way I've done for removed images/tags: put the info in the parameters hash and update the recipient/tracker to point to a parent (e.g. in this case the namespace containing said image).

namespace_name: repository.namespace.clean_name,
tag_name: name
}
)
end
end
Loading