Skip to content

Conversation

@ckunki
Copy link
Contributor

@ckunki ckunki commented Aug 6, 2025

Closes #220

@ckunki ckunki temporarily deployed to manual-approval August 6, 2025 12:54 — with GitHub Actions Inactive
@ckunki ckunki temporarily deployed to text-ai-prerelease August 6, 2025 13:09 — with GitHub Actions Inactive
if tag.startswith(f"{image_name}:{self.flavor}")
]

def clean_all_images(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Add prefix and rename method
  • verify semantics of docker_tag_prefix

Proposal

flavors = (ScriptLanguageContainer.flavor_path,)
clean_flavor_images(
    output_directory=str(self.workspace.output_path),
    flavor=flavors
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you will need to use

with current_directory(self.session.checkout_dir):

Copy link
Contributor Author

@ckunki ckunki Aug 7, 2025

Choose a reason for hiding this comment

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

I think we don't need with current_directory(self.session.checkout_dir):

  • Workspace.output_path is defined self.root_dir / "output"
  • ScriptLanguageContainer.__init__() initializes Workspace with Workspace(Path.cwd())
  • Path.cwd() returns an absolute path

Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the exaslct api calls which use the argument flavor_path unfortunately only wors if the flavor path is relative to the cloned repository. Maybe clean_flavor_images works with any directory, not sure.

Copy link
Contributor Author

@ckunki ckunki Aug 11, 2025

Choose a reason for hiding this comment

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

Please correct me if I'm wrong: flavor_path is still relative to the cloned directory:

@property
def flavor_path(self) -> Path:
    return self.checkout_dir / constants.FLAVORS_PATH_IN_SLC_REPO / self.flavor



def expected_activation_key(slc: ScriptLanguageContainer) -> str:
alias = slc.language_alias
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, if we use the values from slc for testing, we don't actually test if they have the expected values. I mean, there could be something wrong with slc.language_alias. Wouldn't it be better if test for the real name? Or do we have already an unit test for property language_alias?

Copy link
Contributor Author

@ckunki ckunki Aug 11, 2025

Choose a reason for hiding this comment

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

The implementation of property is rather trivial:

    @property
    def language_alias(self) -> str:
        return f"custom_slc_{self.name}"

The actual value of the language alias IMHO is not important.
But, as talking about it, it comes to my mind that it should be unique, for example.
The correctness of the language alias is verified by integration test test_udf_with_custom_packages()

Please tell me, if you still request adding a unit test.

In this case, I will add verifying the uniqueness depending on the SLC's name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a unit test verifying the expected language_alias.

@ckunki ckunki temporarily deployed to manual-approval August 11, 2025 11:25 — with GitHub Actions Inactive
@ckunki ckunki temporarily deployed to text-ai-prerelease August 11, 2025 11:26 — with GitHub Actions Inactive
Co-authored-by: Thomas Ubensee <34603111+tomuben@users.noreply.github.com>
@ckunki ckunki temporarily deployed to text-ai-prerelease August 11, 2025 12:00 — with GitHub Actions Inactive
Comment on lines 90 to 91
with slc_factory.context(sample_slc_name, flavor) as slc:
testee = slc
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
with slc_factory.context(sample_slc_name, flavor) as slc:
testee = slc
with slc_factory.context(sample_slc_name, flavor) as testee:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case you need to add pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See last push.

@ckunki ckunki temporarily deployed to manual-approval August 11, 2025 14:38 — with GitHub Actions Inactive
@ckunki ckunki temporarily deployed to text-ai-prerelease August 11, 2025 14:44 — with GitHub Actions Inactive
@sonarqubecloud
Copy link

@ckunki ckunki merged commit 689ecd3 into main Aug 11, 2025
25 checks passed
@ckunki ckunki deleted the feature/#220-Refactored_class_SlctManager branch August 11, 2025 15:11
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.

SLC UX Gen 2

4 participants